Re: [PATCH 6/7] omap: iommu: remove unused exported API
Hi Ohad, On Thu, Aug 18, 2011 at 2:01 PM, Ohad Ben-Cohen wrote: > Hi Hiroshi, > > On Thu, Aug 18, 2011 at 1:49 PM, Hiroshi DOYU wrote: >>> -extern void iommu_set_twl(struct iommu *obj, bool on); >> >> This function was introduced by "Hari Kanigeri" for some OMAP4 case, >> which is only using TLB, not H/W table walk. > > We discussed that internally, and concluded it's not needed anymore. I > still kept the underlying plumbing mechanism, just in case it'd still > be needed for some hw quirks in the future, but removed the interface > (in case a future hw revision will seem to need it, it would probably > make more sense to invoke the relevant mechanism from the underlying > layers rather than from a higher layer). > >>> -extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end); >> >> This function was introduced by "David Cohen" to specify the available >> ISP virtual address range. This function wasn't actually introduced by me, but I remember to refer it when I was discussing to not define address 0x0 as valid in da range by default (unless intentionally specified by client). > > Not sure if David or anyone is using this, but if someone is, it must > be out-of-tree. I am fine to remove it. Br, David > > In that case, even if it is needed, adding a mainline user for this > function at this point will most likely not be accepted anymore, and > instead, one will be suggested to use (and extend as necessary) the > DMA-API. > > Thanks, > Ohad. > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area
Hi,
On Fri, Oct 01, 2010 at 05:08:53AM +0200, ext Fernando Guzman Lugo wrote:
> As da_end does not belongs to the area the roundup should
> be done to da_end and not to da_end + 1.
> Also the end check for the last area should be
> ULONG_MAX - start + 1 >= bytes.
>
> Signed-off-by: Fernando Guzman Lugo
> ---
> arch/arm/plat-omap/iovmm.c |6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 24ca9c4..fc6b109 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -289,19 +289,19 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
> *obj, u32 da,
> prev_end = 0;
> list_for_each_entry(tmp, &obj->mmap, list) {
>
> - if (prev_end >= start)
> + if (prev_end > start)
> break;
>
> if (start + bytes <= tmp->da_start)
> goto found;
>
> if (flags & IOVMF_DA_ANON)
> - start = roundup(tmp->da_end + 1, alignement);
> + start = roundup(tmp->da_end, alignement);
There's a lack of comment here, but the purpose of tmp->da_end + 1 is to
create a gap between iovm areas to force to trigger iommu faults when
some access exceeds a valid area. Without this gap, such situation
may produce data corruption which is much more difficult to track.
Br,
David
>
> prev_end = tmp->da_end;
> }
>
> - if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> + if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
> goto found;
>
> dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] OMAP3: DMA: Errata: sDMA FIFO draining does not finish
Hi,
On Fri, Oct 01, 2010 at 01:04:55PM +0200, ext Peter Ujfalusi wrote:
> Implement the suggested workaround for OMAP3 regarding to sDMA draining
> issue, when the channel is disabled on the fly.
> This errata affects the following configuration:
> sDMA transfer is source synchronized
> Buffering is enabled
> SmartStandby is selected.
>
> The issue can be easily reproduced by creating overrun situation while
> recording audio.
> Either introduce load to the CPU:
> nice -19 arecord -D hw:0 -M -B 1 -F 5000 -f dat > /dev/null & \
> dd if=/dev/urandom of=/dev/null
>
> or suspending the arecord, and resuming it:
> arecord -D hw:0 -M -B 1 -F 5000 -f dat > /dev/null
> CTRL+Z; fg; CTRL+Z; fg; ...
>
> In case of overrun audio stops DMA, and restarts it (without reseting
> the sDMA channel). When we hit this errata in stop case (sDMA drain did
> not complete), at the coming start the sDMA will not going to be
> operational (it is still draining).
> This leads to DMA stall condition.
> On OMAP3 we can recover with sDMA channel reset, it has been observed
> that by introducing unrelated sDMA activity might also help (reading
> from MMC for example).
>
> The same errata exists for OMAP2, where the suggestion is to disable the
> buffering to avoid this type of error.
> On OMAP3 the suggestion is to set sDMA to NoStandby before disabling
> the channel, and wait for the drain to finish, than configure sDMA to
> SmartStandby again.
>
> Signed-off-by: Peter Ujfalusi
> ---
> arch/arm/plat-omap/dma.c | 35 +++-
> arch/arm/plat-omap/include/plat/dma.h |3 ++
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> index 7115884..287a00e 100644
> --- a/arch/arm/plat-omap/dma.c
> +++ b/arch/arm/plat-omap/dma.c
[snip]
> + l = sys_cf = dma_read(OCP_SYSCONFIG);
Shouldn't it be avoided?
Regards,
David
> + l &= ~DMA_SYSCONFIG_MIDLEMODE_MASK;
> + l |= DMA_SYSCONFIG_MIDLEMODE(DMA_IDLEMODE_NO_IDLE);
> + dma_write(l , OCP_SYSCONFIG);
> +
> + l = dma_read(CCR(lch));
> + l &= ~OMAP_DMA_CCR_EN;
> + dma_write(l, CCR(lch));
> +
> + /* Wait for sDMA FIFO drain */
> + l = dma_read(CCR(lch));
> + while (i < 100 && (l & (OMAP_DMA_CCR_RD_ACTIVE |
> + OMAP_DMA_CCR_WR_ACTIVE))) {
> + udelay(5);
> + i++;
> + l = dma_read(CCR(lch));
> + }
> + if (i >= 100)
> + printk(KERN_ERR "DMA drain did not completed on "
> + "lch %d\n", lch);
> + /* Restore OCP_SYSCONFIG */
> + dma_write(sys_cf, OCP_SYSCONFIG);
> + } else {
> + l &= ~OMAP_DMA_CCR_EN;
> + dma_write(l, CCR(lch));
> + }
>
> if (!omap_dma_in_1510_mode() && dma_chan[lch].next_lch != -1) {
> int next_lch, cur_lch = lch;
> diff --git a/arch/arm/plat-omap/include/plat/dma.h
> b/arch/arm/plat-omap/include/plat/dma.h
> index 776ba44..cf66f85 100644
> --- a/arch/arm/plat-omap/include/plat/dma.h
> +++ b/arch/arm/plat-omap/include/plat/dma.h
> @@ -335,6 +335,9 @@
> #define OMAP2_DMA_MISALIGNED_ERR_IRQ (1 << 11)
>
> #define OMAP_DMA_CCR_EN (1 << 7)
> +#define OMAP_DMA_CCR_RD_ACTIVE (1 << 9)
> +#define OMAP_DMA_CCR_WR_ACTIVE (1 << 10)
> +#define OMAP_DMA_CCR_SEL_SRC_DST_SYNC(1 << 24)
> #define OMAP_DMA_CCR_BUFFERING_DISABLE (1 << 25)
>
> #define OMAP_DMA_DATA_TYPE_S80x00
> --
> 1.7.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] OMAP3: DMA: Errata: sDMA FIFO draining does not finish
On Fri, Oct 01, 2010 at 02:16:53PM +0200, Peter Ujfalusi wrote: > On Friday 01 October 2010 14:50:37 David Cohen wrote: > > > > > + l = sys_cf = dma_read(OCP_SYSCONFIG); > > > > Shouldn't it be avoided? > > I'm restoring the OCP_SYSCONFIG register later on, and reusing 'l' here to > avoid > confusing expression when moving the sDMA to NoStandby. > I could as well introduce other variable for this... No problem with those two variables, but you're doing multiple assignments in one line. You could do something like this: l = dma_read(OCP_SYSCONFIG); sys_cf = l; Regards, David -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area
On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, Fernando wrote:
>
[snip]
> > > arch/arm/plat-omap/iovmm.c |6 +++---
> > > 1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm/plat-omap/iovmm.c
> > b/arch/arm/plat-omap/iovmm.c
> > > index 24ca9c4..fc6b109 100644
> > > --- a/arch/arm/plat-omap/iovmm.c
> > > +++ b/arch/arm/plat-omap/iovmm.c
> > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > *alloc_iovm_area(struct iommu *obj, u32 da,
> > > prev_end = 0;
> > > list_for_each_entry(tmp, &obj->mmap, list) {
> > >
> > > - if (prev_end >= start)
> > > + if (prev_end > start)
> > > break;
> > >
> > > if (start + bytes <= tmp->da_start)
> > > goto found;
> > >
> > > if (flags & IOVMF_DA_ANON)
> > > - start = roundup(tmp->da_end + 1, alignement);
> > > + start = roundup(tmp->da_end, alignement);
> >
> > There's a lack of comment here, but the purpose of
> > tmp->da_end + 1 is to create a gap between iovm areas to
> > force to trigger iommu faults when some access exceeds a
> > valid area. Without this gap, such situation may produce data
> > corruption which is much more difficult to track.
>
> That only works when you are accessing sequencially beyond the
> End of the vm_area. However if you are accessing a random address
> Which is in the mmu tables you still can corrupt memory which does
> Not belong to you. That looks not very effective then why waste
> Memory?
The main intention is to detect sequential access beyond the end of the
vm area and it is effective for that purpose.
i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, responsible
to produce statistics data for the captured image, to write more data than
it should. The workaround described in the errata wasn't enough to avoid
error conditions, so a different approach was implemented. This gap
did help me to make sure the new workaround is valid and no data
corruption was occurring anymore.
Anyway, I can't see why memory is being wasted.
>
> Maybe other mechanism should be implemente like in the process
> Switching when if the process has DMM virtual memory area and if
> So enablig only that area (all other process areas will be
> Dissabled and it would get a mmufault in case of access). However
> That increase the time of switching between process.
Sure. We can have other mechanisms. But in the above scenario, H3A
submodule has sequential access and can corrupt its own data. It has
more than one buffer and they're likely to be mapped to sequential
memory areas without the mechanism you're about to remove.
If you're able to implement a better mechanism, please remove this one
just when a new is already there. :)
Regards,
David
>
> Regards,
> Fernando.
>
> >
> > Br,
> >
> > David
> >
> > >
> > > prev_end = tmp->da_end;
> > > }
> > >
> > > - if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > + if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
> > > goto found;
> > >
> > > dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> > > --
> > > 1.6.3.3
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > linux-kernel" in the body of a message to majord...@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area
On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, Fernando wrote:
>
> > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo,
> > Fernando wrote:
> > >
> >
> > [snip]
> >
> > > > > arch/arm/plat-omap/iovmm.c |6 +++---
> > > > > 1 files changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm/plat-omap/iovmm.c
> > > > b/arch/arm/plat-omap/iovmm.c
> > > > > index 24ca9c4..fc6b109 100644
> > > > > --- a/arch/arm/plat-omap/iovmm.c
> > > > > +++ b/arch/arm/plat-omap/iovmm.c
> > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > > > *alloc_iovm_area(struct iommu *obj, u32 da,
> > > > > prev_end = 0;
> > > > > list_for_each_entry(tmp, &obj->mmap, list) {
> > > > >
> > > > > - if (prev_end >= start)
> > > > > + if (prev_end > start)
> > > > > break;
> > > > >
> > > > > if (start + bytes <= tmp->da_start)
> > > > > goto found;
> > > > >
> > > > > if (flags & IOVMF_DA_ANON)
> > > > > - start = roundup(tmp->da_end +
> > 1, alignement);
> > > > > + start = roundup(tmp->da_end,
> > alignement);
> > > >
> > > > There's a lack of comment here, but the purpose of
> > > > tmp->da_end + 1 is to create a gap between iovm areas to
> > > > force to trigger iommu faults when some access exceeds a
> > valid area.
> > > > Without this gap, such situation may produce data
> > corruption which
> > > > is much more difficult to track.
> > >
> > > That only works when you are accessing sequencially beyond
> > the End of
> > > the vm_area. However if you are accessing a random address
> > Which is in
> > > the mmu tables you still can corrupt memory which does Not
> > belong to
> > > you. That looks not very effective then why waste Memory?
> >
> > The main intention is to detect sequential access beyond the
> > end of the vm area and it is effective for that purpose.
> > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule,
> > responsible to produce statistics data for the captured
> > image, to write more data than it should. The workaround
> > described in the errata wasn't enough to avoid error
> > conditions, so a different approach was implemented. This gap
> > did help me to make sure the new workaround is valid and no
> > data corruption was occurring anymore.
> > Anyway, I can't see why memory is being wasted.
> >
>
> I was taking about vitual memory waste (maybe not so important).
> Is ok for me then keep the gap. Do other changes look good to
> You?
Do you mean in this patch?
All changes make sense only if you're removing the gap, except for the
fix below.
[snip]
> > > > >
> > > > > prev_end = tmp->da_end;
> > > > > }
> > > > >
> > > > > - if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > > > + if ((start >= prev_end) && (ULONG_MAX - start +
> > 1 >= bytes))
This fix is partially valid. The correct change must be only:
- if ((start > prev_end) && (ULONG_MAX - start >= bytes))
+ if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))
Otherwise you wouldn't guarantee the gap for fixed da.
Br,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 1/6] omap3: Add external VBUS power switch and overcurrent detect on IGEP v2 board.
Hi,
On Fri, Oct 01, 2010 at 11:09:04PM +0200, ext Enric Balletbo i Serra wrote:
> GPIO for various devices are missing from the board initialization.
> This patch adds support for the VBUS and over current gpios. Without this
> patch, input/outputs from these two sources are ignored.
>
> Signed-off-by: Enric Balletbo i Serra
> ---
> arch/arm/mach-omap2/board-igep0020.c | 16
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-igep0020.c
> b/arch/arm/mach-omap2/board-igep0020.c
> index 175f043..07dbdb7 100644
> --- a/arch/arm/mach-omap2/board-igep0020.c
> +++ b/arch/arm/mach-omap2/board-igep0020.c
> @@ -274,6 +274,22 @@ static int igep2_twl_gpio_setup(struct device *dev,
> igep2_vmmc1_supply.dev = mmc[0].dev;
> igep2_vmmc2_supply.dev = mmc[1].dev;
>
> + /*
> + * REVISIT: need ehci-omap hooks for external VBUS
> + * power switch and overcurrent detect
> + */
> + if ((gpio_request(gpio + 1, "GPIO_EHCI_NOC") < 0) ||
> + (gpio_direction_input(gpio + 1) < 0))
> + pr_err("IGEP2: Could not obtain gpio for EHCI NOC");
Shouldb't you return error if it can't request the GPIO?
> +
> + /*
> + * TWL4030_GPIO_MAX + 0 == ledA, GPIO_USBH_CPEN
> + * (out, active low)
> + */
> + if ((gpio_request(gpio + TWL4030_GPIO_MAX, "GPIO_USBH_CPEN") < 0) ||
> + (gpio_direction_output(gpio + TWL4030_GPIO_MAX, 0) < 0))
> + pr_err("IGEP2: Could not obtain gpio for USBH_CPEN");
Same here.
> +
> return 0;
> };
>
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area
Hi,
I have no access to my @nokia.com e-mail at this moment, so I'm
replying using my personal one.
On Mon, Oct 4, 2010 at 6:17 AM, Guzman Lugo, Fernando
wrote:
>
>> On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, Fernando wrote:
>> >
>> > > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo,
>> > > Fernando wrote:
>> > > >
>> > >
>> > > [snip]
>> > >
>> > > > > > arch/arm/plat-omap/iovmm.c | 6 +++---
>> > > > > > 1 files changed, 3 insertions(+), 3 deletions(-)
>> > > > > >
>> > > > > > diff --git a/arch/arm/plat-omap/iovmm.c
>> > > > > b/arch/arm/plat-omap/iovmm.c
>> > > > > > index 24ca9c4..fc6b109 100644
>> > > > > > --- a/arch/arm/plat-omap/iovmm.c
>> > > > > > +++ b/arch/arm/plat-omap/iovmm.c
>> > > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
>> > > > > *alloc_iovm_area(struct iommu *obj, u32 da,
>> > > > > > prev_end = 0;
>> > > > > > list_for_each_entry(tmp, &obj->mmap, list) {
>> > > > > >
>> > > > > > - if (prev_end >= start)
>> > > > > > + if (prev_end > start)
>> > > > > > break;
>> > > > > >
>> > > > > > if (start + bytes <= tmp->da_start)
>> > > > > > goto found;
>> > > > > >
>> > > > > > if (flags & IOVMF_DA_ANON)
>> > > > > > - start = roundup(tmp->da_end +
>> > > 1, alignement);
>> > > > > > + start = roundup(tmp->da_end,
>> > > alignement);
>> > > > >
>> > > > > There's a lack of comment here, but the purpose of
>> > > > > tmp->da_end + 1 is to create a gap between iovm areas to
>> > > > > force to trigger iommu faults when some access exceeds a
>> > > valid area.
>> > > > > Without this gap, such situation may produce data
>> > > corruption which
>> > > > > is much more difficult to track.
>> > > >
>> > > > That only works when you are accessing sequencially beyond
>> > > the End of
>> > > > the vm_area. However if you are accessing a random address
>> > > Which is in
>> > > > the mmu tables you still can corrupt memory which does Not
>> > > belong to
>> > > > you. That looks not very effective then why waste Memory?
>> > >
>> > > The main intention is to detect sequential access beyond the
>> > > end of the vm area and it is effective for that purpose.
>> > > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule,
>> > > responsible to produce statistics data for the captured
>> > > image, to write more data than it should. The workaround
>> > > described in the errata wasn't enough to avoid error
>> > > conditions, so a different approach was implemented. This gap
>> > > did help me to make sure the new workaround is valid and no
>> > > data corruption was occurring anymore.
>> > > Anyway, I can't see why memory is being wasted.
>> > >
>> >
>> > I was taking about vitual memory waste (maybe not so important).
>> > Is ok for me then keep the gap. Do other changes look good to
>> > You?
>>
>> Do you mean in this patch?
>> All changes make sense only if you're removing the gap, except for the
>> fix below.
>
> The thing is, the dspbridge needs to map some register in order to DSP
> can read and configure some of them. We need to map some pages
> with fix addresses and to do that I use iommu_kmap. So when some
> of that pages are contiguous I get his error:
>
> "%s: no space to fit %08x(%x) flags: %08x\n"
>
> Which is not true. The page to page perfectly fix, but the check with 1 byte
> more avoid that it could be mapped and I am getting the error.
>
> I am not agree with the gap, but I am ok when it is not fixed address as
> below code
>
> if (flags & IOVMF_DA_ANON)
> start = roundup(tmp->da_end + 1, alignement);
>
> But it is breaking the tidspbridge when the gap is used for fixed addresses.
>
> It should not fail when we want to map a page what is freed just because of
> the gap.
> Please let me know what you thing.
I got your point. I agree the gap shouldn't be forced for fixed da.
IMO you can apply this change when !(flags & IOVMF_DA_ANON).
Regards,
David
>
> Thanks,
> Fernando.
>
>>
>> [snip]
>>
>> > > > > >
>> > > > > > prev_end = tmp->da_end;
>> > > > > > }
>> > > > > >
>> > > > > > - if ((start > prev_end) && (ULONG_MAX - start >= bytes))
>> > > > > > + if ((start >= prev_end) && (ULONG_MAX - start +
>> > > 1 >= bytes))
>>
>> This fix is partially valid. The correct change must be only:
>> - if ((start > prev_end) && (ULONG_MAX - start >= bytes))
>> + if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))
>>
>> Otherwise you wouldn't guarantee the gap for fixed da.
>>
>> Br,
>>
>> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at htt
Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area
On Mon, Oct 04, 2010 at 05:37:04PM +0200, ext Guzman Lugo, Fernando wrote:
>
>
> > -Original Message-
> > From: David Cohen [mailto:daco...@gmail.com]
> > Sent: Monday, October 04, 2010 7:11 AM
> > To: Guzman Lugo, Fernando
> > Cc: David Cohen; Doyu Hiroshi (Nokia-MS/Espoo); Contreras
> > Felipe (Nokia-MS/Helsinki); Palande Ameya
> > (Nokia-MS/Helsinki); linux-ker...@vger.kernel.org;
> > andy.shevche...@gmail.com; linux-omap@vger.kernel.org
> > Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and
> > end check for the last area
> >
> > Hi,
> >
> > I have no access to my @nokia.com e-mail at this moment, so
> > I'm replying using my personal one.
> >
> > On Mon, Oct 4, 2010 at 6:17 AM, Guzman Lugo, Fernando
> > wrote:
> > >
> > >> On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo,
> > Fernando wrote:
> > >> >
> > >> > > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo,
> > >> > > Fernando wrote:
> > >> > > >
> > >> > >
> > >> > > [snip]
> > >> > >
> > >> > > > > > arch/arm/plat-omap/iovmm.c | 6 +++---
> > >> > > > > > 1 files changed, 3 insertions(+), 3 deletions(-)
> > >> > > > > >
> > >> > > > > > diff --git a/arch/arm/plat-omap/iovmm.c
> > >> > > > > b/arch/arm/plat-omap/iovmm.c
> > >> > > > > > index 24ca9c4..fc6b109 100644
> > >> > > > > > --- a/arch/arm/plat-omap/iovmm.c
> > >> > > > > > +++ b/arch/arm/plat-omap/iovmm.c
> > >> > > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > >> > > > > *alloc_iovm_area(struct iommu *obj, u32 da,
> > >> > > > > > prev_end = 0;
> > >> > > > > > list_for_each_entry(tmp, &obj->mmap, list) {
> > >> > > > > >
> > >> > > > > > - if (prev_end >= start)
> > >> > > > > > + if (prev_end > start)
> > >> > > > > > break;
> > >> > > > > >
> > >> > > > > > if (start + bytes <= tmp->da_start)
> > >> > > > > > goto found;
> > >> > > > > >
> > >> > > > > > if (flags & IOVMF_DA_ANON)
> > >> > > > > > - start = roundup(tmp->da_end +
> > >> > > 1, alignement);
> > >> > > > > > + start = roundup(tmp->da_end,
> > >> > > alignement);
> > >> > > > >
> > >> > > > > There's a lack of comment here, but the purpose of
> > >> > > > > tmp->da_end + 1 is to create a gap between iovm areas to
> > >> > > > > force to trigger iommu faults when some access exceeds a
> > >> > > valid area.
> > >> > > > > Without this gap, such situation may produce data
> > >> > > corruption which
> > >> > > > > is much more difficult to track.
> > >> > > >
> > >> > > > That only works when you are accessing sequencially beyond
> > >> > > the End of
> > >> > > > the vm_area. However if you are accessing a random address
> > >> > > Which is in
> > >> > > > the mmu tables you still can corrupt memory which does Not
> > >> > > belong to
> > >> > > > you. That looks not very effective then why waste Memory?
> > >> > >
> > >> > > The main intention is to detect sequential access
> > beyond the end
> > >> > > of the vm area and it is effective for that purpose.
> > >> > > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule,
> > >> > > responsible to produce statistics data for the
> > captured image, to
> > >> > > write more data than it should. The workaround
> > described in the
> > >> > > errata wasn't enough to avoid error conditions, so a different
> > >> > > approach was implemented. This gap did help me to make
> > sure the
> > >> &
Re: [PATCH v2 1/1] OMAP: IOMMU: add support to callback during fault handling
On Tue, Feb 15, 2011 at 5:10 PM, David Cohen wrote:
> On Tue, Feb 15, 2011 at 4:48 PM, Hiroshi DOYU wrote:
>> Hi David,
>
> Hi Hiroshi,
>
>>
>> Sorry for a bit late reply
>
> You're just in time. :)
>
>>
>> From: David Cohen
>> 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
>>> ---
>>> 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;
>>
[PATCH v3 0/2] OMAP: IOMMU fault callback support
Hi, This patch set adapts current (*isr)() to be used as fault callback. IOMMU faults might be very difficult to reproduce and then to figure out the source of the problem. Currently IOMMU driver prints not so useful debug message and does not notice user about such issue. With a fault callback, IOMMU user may debug much more useful information and/or react to go back to a valid state. Br, David --- David Cohen (2): OMAP2+: IOMMU: don't print fault warning on specific layer OMAP: IOMMU: add support to callback during fault handling arch/arm/mach-omap2/iommu2.c| 33 +--- arch/arm/plat-omap/include/plat/iommu.h | 14 - arch/arm/plat-omap/iommu.c | 52 ++- 3 files changed, 65 insertions(+), 34 deletions(-) -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] OMAP2+: IOMMU: don't print fault warning on specific layer
IOMMU upper layer and user are responsible to handle a fault and to
define whether it will end up as an error or not. OMAP2+ specific
layer should not print anything in such case.
Signed-off-by: David Cohen
---
arch/arm/mach-omap2/iommu2.c | 16
1 files changed, 0 insertions(+), 16 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..49a1e5e 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -145,15 +145,7 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
{
- int i;
u32 stat, da;
- const char *err_msg[] = {
- "tlb miss",
- "translation fault",
- "emulation miss",
- "table walk fault",
- "multi hit fault",
- };
stat = iommu_read_reg(obj, MMU_IRQSTATUS);
stat &= MMU_IRQ_MASK;
@@ -163,14 +155,6 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32
*ra)
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");
-
iommu_write_reg(obj, stat, MMU_IRQSTATUS);
return stat;
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Add support to register an isr for IOMMU fault situations and adapt it
to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
module might want to be informed when errors happen in order to debug it
or react.
Signed-off-by: David Cohen
---
arch/arm/mach-omap2/iommu2.c| 17 +-
arch/arm/plat-omap/include/plat/iommu.h | 14 -
arch/arm/plat-omap/iommu.c | 52 ++-
3 files changed, 65 insertions(+), 18 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 49a1e5e..adb083e 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool
on)
static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
{
u32 stat, da;
+ u32 errs = 0;
stat = iommu_read_reg(obj, MMU_IRQSTATUS);
stat &= MMU_IRQ_MASK;
- if (!stat)
+ if (!stat) {
+ *ra = 0;
return 0;
+ }
da = iommu_read_reg(obj, MMU_FAULT_AD);
*ra = da;
+ 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_write_reg(obj, stat, MMU_IRQSTATUS);
- return stat;
+ return errs;
}
static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
diff --git a/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 19cbb5e..174f1b9 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*isr_priv;
unsigned intrefcount;
struct mutexiommu_lock; /* global for this whole object */
@@ -47,7 +48,7 @@ struct iommu {
struct list_headmmap;
struct mutexmmap_lock; /* protect mmap */
- int (*isr)(struct iommu *obj);
+ int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
void *ctx; /* iommu context: registres saved area */
u32 da_start;
@@ -109,6 +110,13 @@ struct iommu_platform_data {
u32 da_end;
};
+/* IOMMU errors */
+#define OMAP_IOMMU_ERR_TLB_MISS(1 << 0)
+#define OMAP_IOMMU_ERR_TRANS_FAULT (1 << 1)
+#define OMAP_IOMMU_ERR_EMU_MISS(1 << 2)
+#define OMAP_IOMMU_ERR_TBLWALK_FAULT (1 << 3)
+#define OMAP_IOMMU_ERR_MULTIHIT_FAULT (1 << 4)
+
#if defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet"
#else
@@ -161,6 +169,10 @@ extern size_t iopgtable_clear_entry(struct iommu *obj, u32
iova);
extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end);
extern struct iommu *iommu_get(const char *name);
extern void iommu_put(struct iommu *obj);
+extern int iommu_set_isr(const char *name,
+int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
+ void *priv),
+void *isr_priv);
extern void iommu_save_ctx(struct iommu *obj);
extern void iommu_restore_ctx(struct iommu *obj);
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index f55f458..b0e0efc 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -780,25 +780,19 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
*/
static irqreturn_t iommu_fault_handler(int irq, void *data)
{
- u32 stat, da;
+ u32 da, errs;
u32 *iopgd, *iopte;
- int err = -EIO;
struct iommu *obj = 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);
+ errs = iommu_report_fault(obj, &da);
clk_disable(obj->clk);
- if (!stat)
+
+ /* Fault callback or TLB/PTE Dynamic loading */
+ if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv))
return IRQ_HANDLED;
iommu_disable(obj);
@@ -806,15 +800,16 @@ static irqreturn_t iommu_fault_handler(int irq, void
*data)
iopgd = iopgd_offset(obj, da);
if (!iopgd_is_table(*iopgd)) {
- dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n", obj->
Re: [PATCH 10/12] mfd: TWL4030: changes for TRITON Errata 27 workaround
ead_u8(TWL4030_MODULE_PM_RECEIVER, &val,
> + smps_osc_reg[i]);
> + val |= EXT_FS_CLK_EN;
> + err |= twl_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, val,
> + smps_osc_reg[i]);
> + }
> +
> + /* restore the original value */
> + err |= twl_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, wdt_counter_val,
> + R_WDT_CFG);
> + if (err)
> + pr_warning("TWL4030: workaround setup failed!\n");
> +}
> +
> +bool is_twl5030_errata27wa_required(void)
> +{
> + u32 twl5030_si_ver;
> + int ret = 0;
> +
> + ret = twl5030_get_si_ver(&twl5030_si_ver);
> + if (ret)
> + pr_warning("TWL4030: Unable to get the Si version\n");
You can get rid of 'ret'. What about:
if (twl5030_get_si_ver(&twl5030_si_ver))
pr_warning("TWL4030: Unable to get the Si version\n");
> +
> + if (twl5030_si_ver < TWL5030_REV_1_2)
> + return true;
> + else
> + return false;
No need to if...else here. You can:
return twl5030_si_ver < TWL5030_REV_1_2;
Regards,
David Cohen
> +}
> +
> void __init twl4030_power_init(struct twl4030_power_data *twl4030_scripts)
> {
> int err = 0;
> @@ -530,6 +592,18 @@ void __init twl4030_power_init(struct twl4030_power_data
> *twl4030_scripts)
> if (err)
> goto unlock;
>
> + /* Applying TWL5030 Errata 27 WA based on Si revision &
> + * flag updated from board file*/
> + if (is_twl5030_errata27wa_required()) {
> + pr_info("TWL5030: Enabling workaround for Si Errata 27\n");
> + twl_errata27_workaround();
> + if (twl4030_scripts->twl5030_errata27wa_vcsetup)
> + twl4030_scripts->twl5030_errata27wa_vcsetup();
> +
> + if (twl4030_scripts->twl5030_errata27wa_script)
> + twl4030_scripts->twl5030_errata27wa_script();
> + }
> +
> for (i = 0; i < twl4030_scripts->num; i++) {
> err = load_twl4030_script(twl4030_scripts->scripts[i],
> address);
> if (err)
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index 5d3f2bf..26232f3 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -667,6 +667,8 @@ struct twl4030_power_data {
> struct twl4030_script **scripts;
> unsigned num;
> struct twl4030_resconfig *resource_config;
> + void (*twl5030_errata27wa_vcsetup)(void);
> + void (*twl5030_errata27wa_script)(void);
> #define TWL4030_RESCONFIG_UNDEF ((u8)-1)
> };
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/12] omap3: pm: Fix for the TRITON sleep/wakeup sequence
Hi,
On Fri, Feb 18, 2011 at 7:08 PM, Lesly A M wrote:
> Since the function to populate the sleep script is getting called always
> irrespective of the flag "TWL4030_SLEEP_SCRIPT", other scripts data
> is getting over written by the sleep script.
Are you sure this is the correct patch description? For me it's something like:
Add missing brackets to if statement.
Br,
David Cohen
>
> Signed-off-by: Lesly A M
> Cc: Nishanth Menon
> Cc: David Derrick
> Cc: Samuel Ortiz
> ---
> drivers/mfd/twl4030-power.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mfd/twl4030-power.c b/drivers/mfd/twl4030-power.c
> index 16422de..2c0d4d1 100644
> --- a/drivers/mfd/twl4030-power.c
> +++ b/drivers/mfd/twl4030-power.c
> @@ -447,12 +447,13 @@ static int __init load_twl4030_script(struct
> twl4030_script *tscript,
> if (err)
> goto out;
> }
> - if (tscript->flags & TWL4030_SLEEP_SCRIPT)
> + if (tscript->flags & TWL4030_SLEEP_SCRIPT) {
> if (order)
> pr_warning("TWL4030: Bad order of scripts (sleep "\
> "script before wakeup) Leads to boot"\
> "failure on some boards\n");
> err = twl4030_config_sleep_sequence(address);
> + }
> out:
> return err;
> }
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/12] omap3: pm: TWL5030 version checking
Hi,
On Fri, Feb 18, 2011 at 7:08 PM, Lesly A M wrote:
> Added api to get the TWL5030 Si version from the IDCODE register.
> It is used for enabling the workaround for TWL errata 27.
>
> Signed-off-by: Lesly A M
> Cc: Nishanth Menon
> Cc: David Derrick
> Cc: Samuel Ortiz
> ---
> drivers/mfd/twl-core.c | 50
> +++
> include/linux/i2c/twl.h | 10 +
> 2 files changed, 60 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index a35fa7d..211c2cc 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -487,6 +487,56 @@ EXPORT_SYMBOL(twl_i2c_read_u8);
>
> /*--*/
>
> +/**
> + * twl_read_idcode_register - api to read the IDCODE register.
> + * @value: returns 32 bit IDCODE register value.
> + *
> + * Unlocks the IDCODE register and read the 32 bit value.
> + */
> +int twl_read_idcode_register(u32 *value)
> +{
> + int err = 0;
No need to initialize 'ret'.
> +
> + err = twl_i2c_write_u8(TWL4030_MODULE_INTBR, TWL_EEPROM_R_UNLOCK,
> + R_UNLOCK_TEST_REG);
> + if (err)
> + pr_err("TWL4030 Unable to unlock IDCODE registers\n");
> +
> + err = twl_i2c_read(TWL4030_MODULE_INTBR, (u8 *)(value),
> + 0x0, 4);
Variable 'err' is being overwritten here.
> + if (err)
> + pr_err("TWL4030: unable to read IDCODE-%d\n", err);
> +
> + err = twl_i2c_write_u8(TWL4030_MODULE_INTBR, 0x0, R_UNLOCK_TEST_REG);
Here again.
> + if (err)
> + pr_err("TWL4030 Unable to relock IDCODE registers\n");
> +
> + return err;
And then you're returning 'err'. Unless you care only about the last
'err' assignment, something is wrong here, isn't it? :)
> +}
> +
> +/**
> + * twl5030_get_si_ver - api to get TWL5030 Si version.
> + * @value: returns Si version from IDCODE.
> + *
> + * Api to get the TWL5030 Si version from IDCODE value.
> + */
> +int twl5030_get_si_ver(u32 *value)
> +{
> + int ret = 0;
No need to initialize 'ret'.
> + static u32 twl_idcode;
> +
> + if (twl_idcode == 0)
> + ret = twl_read_idcode_register(&twl_idcode);
> + if (ret)
> + pr_err("TWL4030 Unable to check Si version\n");
Shouldn't you return error here?
> +
> + if (TWL_SIL_TYPE(twl_idcode) == TWL_SIL_5030)
> + *value = TWL_SIL_REV(twl_idcode);
Otherwise, what happens here if twl_read_idcode_register() fails?
> +
> + return ret;
return 0 if the code reaches this point.
Br,
David Cohen
> +}
> +EXPORT_SYMBOL(twl5030_get_si_ver);
> +
> static struct device *
> add_numbered_child(unsigned chip, const char *name, int num,
> void *pdata, unsigned pdata_len,
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index f4bd475..5d3f2bf 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -150,7 +150,15 @@
> #define MMC_PU (0x1 << 3)
> #define MMC_PD (0x1 << 2)
>
> +#define R_UNLOCK_TEST_REG 0x12
> +#define TWL_EEPROM_R_UNLOCK 0x49
>
> +#define TWL_SIL_TYPE(rev) ((rev) & 0x00FF)
> +#define TWL_SIL_REV(rev) ((rev) >> 24)
> +#define TWL_SIL_5030 0x09002F
> +#define TWL5030_REV_1_0 0x00
> +#define TWL5030_REV_1_1 0x10
> +#define TWL5030_REV_1_2 0x30
>
> #define TWL4030_CLASS_ID 0x4030
> #define TWL6030_CLASS_ID 0x6030
> @@ -180,6 +188,8 @@ int twl_i2c_read_u8(u8 mod_no, u8 *val, u8 reg);
> int twl_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes);
> int twl_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes);
>
> +int twl5030_get_si_ver(u32 *value);
> +
> int twl6030_interrupt_unmask(u8 bit_mask, u8 offset);
> int twl6030_interrupt_mask(u8 bit_mask, u8 offset);
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] OMAP: Voltage: Adaptive Body-Bias handlers
t; + }
> + }
> +
> + if (timeout == ABB_TRANXDONE_TIMEOUT) {
> + pr_warning("%s: TRANXDONE timed out trying to clear status\n",
> + __func__);
> + ret = -EBUSY;
return -EBUSY.
> + }
> +
> + return ret;
return 0
> +}
> +
> /* OMAP3 specific voltage init functions */
>
> /*
> @@ -789,6 +1009,64 @@ static void __init omap3_vc_init(struct omap_vdd_info
> *vdd)
> is_initialized = true;
> }
>
> +/**
> + * omap3_abb_configure - per-VDD configuration of ABB
> + *
> + * @abb : abb instance being initialized
> + */
> +static int omap3_abb_configure(struct omap_abb_info *abb)
> +{
> + int ret = 0;
Once more you don't need 'ret'.
> + u32 sr2_wt_cnt_val;
> + struct clk *sys_ck;
> + struct omap_vdd_info *vdd;
> +
> + if (!abb || IS_ERR(abb)) {
> + pr_warning("%s: invalid abb\n", __func__);
> + ret = -EINVAL;
> + goto out;
Why set 'ret', jump to 'out' just to return 'ret'? Return -EINVAL now.
> + }
> +
> + sys_ck = clk_get(NULL, "sys_ck");
> + if (IS_ERR(sys_ck)) {
> + pr_warning("%s: unable to fetch SYS_CK\n", __func__);
> + ret = -ENODEV;
return -ENODEV
> + goto out;
> + }
> +
> + vdd = container_of(abb, struct omap_vdd_info, abb);
> +
> + /* LDO settling time */
> + sr2_wt_cnt_val = clk_get_rate(sys_ck);
> + sr2_wt_cnt_val = sr2_wt_cnt_val / 100 / 16;
> +
> + omap2_prm_rmw_mod_reg_bits(OMAP3630_SR2_WTCNT_VALUE_MASK,
> + (sr2_wt_cnt_val << OMAP3630_SR2_WTCNT_VALUE_SHIFT),
> + OMAP3430_GR_MOD, abb->setup_offs);
> +
> + /* allow FBB operation */
> + omap2_prm_set_mod_reg_bits(OMAP3630_ACTIVE_FBB_SEL_MASK,
> + OMAP3430_GR_MOD, abb->setup_offs);
> +
> + /* do not allow ACTIVE RBB operation */
> + omap2_prm_set_mod_reg_bits(OMAP3630_ACTIVE_RBB_SEL_MASK,
> + OMAP3430_GR_MOD, abb->setup_offs);
> +
> + /* do not allow SLEEP RBB operation */
> + omap2_prm_set_mod_reg_bits(OMAP3630_SLEEP_RBB_SEL_MASK,
> + OMAP3430_GR_MOD, abb->setup_offs);
> +
> + /* enable ABB LDO */
> + omap2_prm_set_mod_reg_bits(OMAP3630_SR2EN_MASK,
> + OMAP3430_GR_MOD, abb->ctrl_offs);
> +
> + /* register the notifier handler */
> + omap_voltage_register_notifier(vdd, &abb->nb);
> +
> +out:
> + return ret;
return 0
> +}
> +
> /* Sets up all the VDD related info for OMAP3 */
> static int __init omap3_vdd_data_configure(struct omap_vdd_info *vdd)
> {
> @@ -824,6 +1102,9 @@ static int __init omap3_vdd_data_configure(struct
> omap_vdd_info *vdd)
> vdd->vc_reg.smps_volra_mask = OMAP3430_VOLRA0_MASK;
> vdd->vc_reg.voltsetup_shift = OMAP3430_SETUP_TIME1_SHIFT;
> vdd->vc_reg.voltsetup_mask = OMAP3430_SETUP_TIME1_MASK;
> +
> + /* configure ABB */
> + vdd->abb.configure(&vdd->abb);
> } else if (!strcmp(vdd->voltdm.name, "core")) {
> if (cpu_is_omap3630())
> vdd->volt_data = omap36xx_vddcore_volt_data;
> @@ -975,6 +1256,73 @@ static void __init omap4_vc_init(struct omap_vdd_info
> *vdd)
> is_initialized = true;
> }
>
> +/**
> + * omap4_abb_configure - per-VDD configuration of ABB
> + *
> + * @abb : abb instance being initialized
> + */
> +static int omap4_abb_configure(struct omap_abb_info *abb)
> +{
> + int ret = 0;
Get rid of 'ret'.
> + u32 sr2_wt_cnt_val;
> + struct clk *sys_ck;
> + struct omap_vdd_info *vdd;
> +
> + if (!abb || IS_ERR(abb)) {
> + pr_warning("%s: invalid abb\n", __func__);
> + ret = -EINVAL;
return -EINVAL
> + goto out;
> + }
> +
> + sys_ck = clk_get(NULL, "sys_clkin_ck");
> + if (IS_ERR(sys_ck)) {
> + pr_warning("%s: unable to fetch SYS_CK", __func__);
> + ret = -ENODEV;
return -ENODEV
> + goto out;
> + }
> +
> + vdd = container_of(abb, struct omap_vdd_info, abb);
> +
> + /* LDO settling time */
> + sr2_wt_cnt_val = clk_get_rate(sys_ck);
Re: [PATCH 10/12] mfd: TWL4030: changes for TRITON Errata 27 workaround
On Sat, Feb 19, 2011 at 1:33 AM, David Cohen wrote:
> Hi,
>
> On Fri, Feb 18, 2011 at 7:08 PM, Lesly A M wrote:
>> Workaround for TWL5030 Silicon Errata 27 & 28:
>> 27 - VDD1, VDD2, may have glitches when their output value is updated.
>> 28 - VDD1 and / or VDD2 DCDC clock may stop working when internal
>> clock
>> is switched from internal to external.
>>
>> Errata 27:
>> If the DCDC regulators is running on their internal oscillator,
>> negative glitches may occur on VDD1, VDD2 output when voltage is
>> changed.
>> The OMAP device may reboot if the VDD1 or VDD2 go below the
>> core minimum operating voltage.
>>
>> WORKAROUND
>> Set up the TWL5030 DC-DC power supplies to use the HFCLKIN instead of
>> the internal oscillator.
>>
>> Errata 28:
>> VDD1/VDD2 clock system may hang during switching the clock source from
>> internal oscillator to external. VDD1/VDD2 output voltages may
>> collapse
>> if clock stops.
>>
>> WORKAROUND
>> If HFCLK is disabled in OFFMODE, modify the sleep/wakeup sequence and
>> setuptimes to make sure the switching will happen only when HFCLKIN
>> is stable.
>> Also use the TWL5030 watchdog to safeguard the first switching from
>> internal oscillator to HFCLKIN during the TWL5030 init.
>>
>> IMPACT
>> setup time and the power sequence is changed.
>> sleep/wakeup time values will be changed.
>>
>> The workaround changes are called from twl4030_power_init(), since we have to
>> make some i2c_read calls to check the TRITON version & the i2c will not be
>> initialized in the early stage.
>>
>> This workaround is required for TWL5030 Silicon version less than ES1.2
>> The power script & setup time changes are recommended by TI HW team.
>>
>> http://omapedia.org/wiki/TWL4030_power_scripts
>>
>> Changes taken from TRITON Errata27 workaround patch by Nishanth Menon.
>>
>> Signed-off-by: Lesly A M
>> Cc: Nishanth Menon
>> Cc: David Derrick
>> Cc: Samuel Ortiz
>> ---
>> arch/arm/mach-omap2/omap_twl.c | 35 +
>> arch/arm/mach-omap2/twl4030.c | 115
>> +
>> arch/arm/plat-omap/include/plat/voltage.h | 2 +
>> drivers/mfd/twl4030-power.c | 74 ++
>> include/linux/i2c/twl.h | 2 +
>> 5 files changed, 228 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_twl.c b/arch/arm/mach-omap2/omap_twl.c
>> index f0feab9..d6984b8 100644
>> --- a/arch/arm/mach-omap2/omap_twl.c
>> +++ b/arch/arm/mach-omap2/omap_twl.c
>> @@ -176,6 +176,14 @@
>> #define OMAP3_CLKSETUP_RET 31 /* 30.5 uS */
>> #define OMAP3_CLKSETUP_OFF 1 /* 10 mS */
>>
>> +/*
>> + * The clk/volt setuptime is adjusted to do the VDD1/VDD2 voltage rampup
>> + * only after HFCLKIN is stabilized and the HFCLKOUT is enabled
>> + */
>> +#define CLKSETUP_TWL5030_ERRATA27 11567 /* 11.567 mS */
>> +#define VOLTOFFSET_TWL5030_ERRATA27 516 /* 516 uS */
>> +#define VOLTSETUP2_TWL5030_ERRATA27 11079 /* 11.079 mS */
>> +
>> static bool is_offset_valid;
>> static u8 smps_offset;
>>
>> @@ -482,6 +490,33 @@ static struct omap_volt_pmic_info omap4_core_volt_info
>> = {
>> .uv_to_vsel = twl6030_uv_to_vsel,
>> };
>>
>> +/**
>> + * omap3_twl5030_errata27() - update the clk & volt setup time.
>> + *
>> + * Api to update the clk & volt setup time for TWL5030 errata 27.
>> + */
>> +void omap3_twl5030_errata27(void)
>> +{
>> + if (!cpu_is_omap34xx())
>> + return;
>> +
>> + if (cpu_is_omap3630()) {
>> + omap3630_mpu_volt_info.voltsetup_off.voltsetup2 =
>> + VOLTSETUP2_TWL5030_ERRATA27;
>> + omap3630_mpu_volt_info.voltsetup_off.voltoffset =
>> + VOLTOFFSET_TWL5030_ERRATA27;
>> + omap3630_mpu_volt_info.clksetup_off.clksetup =
>> + CLKSETUP_TWL5030_ERRATA27;
>> + } else {
>> + omap3430_mpu_volt_info.voltsetup_off.voltsetup2 =
>> + VOLTSETUP2_TWL5030_ERRATA27;
>> + omap3430_mpu_volt_info.voltsetup_off.voltoff
Re: [PATCH resend] video: omap24xxcam: Fix compilation
Hi Sakari and Felipe, On Tue, Feb 15, 2011 at 2:17 PM, Sakari Ailus wrote: > Felipe Balbi wrote: >> Hi, >> >> On Tue, Feb 15, 2011 at 12:50:12PM +0100, Thomas Weber wrote: >>> Hello Felipe, >>> >>> in include/linux/wait.h >>> >>> #define wake_up(x) __wake_up(x, TASK_NORMAL, 1, NULL) >> >> aha, now I get it, so shouldn't the real fix be including >> on , I mean, it's who uses a symbol >> defined in , right ? That's a tricky situation. linux/sched.h includes indirectly linux/completion.h which includes linux/wait.h. By including sched.h in wait.h, the side effect is completion.h will then include a blank wait.h file and trigger a compilation error every time wait.h is included by any file. > > Surprisingly many other files still don't seem to be affected. But this > is actually a better solution (to include sched.h in wait.h). It does not affect all files include wait.h because TASK_* macros are used with #define statements only. So it has no effect unless some file tries to use a macro which used TASK_*. It seems the usual on kernel is to include both wait.h and sched.h when necessary. IMO your patch is fine. Br, David > > -- > Sakari Ailus > sakari.ai...@maxwell.research.nokia.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH resend] video: omap24xxcam: Fix compilation
On Sat, Feb 19, 2011 at 5:00 PM, Felipe Balbi wrote: > Hi, > > On Sat, Feb 19, 2011 at 01:35:09PM +0200, David Cohen wrote: >> >> aha, now I get it, so shouldn't the real fix be including >> >> on , I mean, it's who uses a symbol >> >> defined in , right ? >> >> That's a tricky situation. linux/sched.h includes indirectly >> linux/completion.h which includes linux/wait.h. > > Ok, so the real problem is that there is circular dependency between > and > >> By including sched.h in wait.h, the side effect is completion.h will >> then include a blank wait.h file and trigger a compilation error every >> time wait.h is included by any file. > > true, but the real problem is the circular dependency between those > files. > >> > Surprisingly many other files still don't seem to be affected. But this >> > is actually a better solution (to include sched.h in wait.h). >> >> It does not affect all files include wait.h because TASK_* macros are >> used with #define statements only. So it has no effect unless some >> file tries to use a macro which used TASK_*. It seems the usual on >> kernel is to include both wait.h and sched.h when necessary. >> IMO your patch is fine. > > I have to disagree. The fundamental problem is the circular dependency > between those two files: > > sched.h uses wait_queue_head_t defined in wait.h > wait.h uses TASK_* defined in sched.h > > So, IMO the real fix would be clear out the circular dependency. Maybe > introducing to define those TASK_* symbols and include > that on sched.h and wait.h > > Just dig a quick and dirty to try it out and works like a charm We have 2 problems: - omap24xxcam compilation broken - circular dependency between sched.h and wait.h To fix the broken compilation we can do what the rest of the kernel is doing, which is to include sched.h. Then, the circular dependency is fixed by some different approach which would probably change *all* current usage of TASK_*. IMO, there's no need to create a dependency between those issues. Br, David > > -- > balbi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/12] mfd: TWL4030: changes for TRITON Errata 27 workaround
On Sat, Feb 19, 2011 at 2:44 AM, David Cohen wrote:
> On Sat, Feb 19, 2011 at 1:33 AM, David Cohen wrote:
>> Hi,
>>
>> On Fri, Feb 18, 2011 at 7:08 PM, Lesly A M wrote:
>>> Workaround for TWL5030 Silicon Errata 27 & 28:
>>> 27 - VDD1, VDD2, may have glitches when their output value is
>>> updated.
>>> 28 - VDD1 and / or VDD2 DCDC clock may stop working when internal
>>> clock
>>> is switched from internal to external.
>>>
>>> Errata 27:
>>> If the DCDC regulators is running on their internal oscillator,
>>> negative glitches may occur on VDD1, VDD2 output when voltage is
>>> changed.
>>> The OMAP device may reboot if the VDD1 or VDD2 go below the
>>> core minimum operating voltage.
>>>
>>> WORKAROUND
>>> Set up the TWL5030 DC-DC power supplies to use the HFCLKIN instead of
>>> the internal oscillator.
>>>
>>> Errata 28:
>>> VDD1/VDD2 clock system may hang during switching the clock source
>>> from
>>> internal oscillator to external. VDD1/VDD2 output voltages may
>>> collapse
>>> if clock stops.
>>>
>>> WORKAROUND
>>> If HFCLK is disabled in OFFMODE, modify the sleep/wakeup sequence and
>>> setuptimes to make sure the switching will happen only when HFCLKIN
>>> is stable.
>>> Also use the TWL5030 watchdog to safeguard the first switching from
>>> internal oscillator to HFCLKIN during the TWL5030 init.
>>>
>>> IMPACT
>>> setup time and the power sequence is changed.
>>> sleep/wakeup time values will be changed.
>>>
>>> The workaround changes are called from twl4030_power_init(), since we have
>>> to
>>> make some i2c_read calls to check the TRITON version & the i2c will not be
>>> initialized in the early stage.
>>>
>>> This workaround is required for TWL5030 Silicon version less than ES1.2
>>> The power script & setup time changes are recommended by TI HW team.
>>>
>>> http://omapedia.org/wiki/TWL4030_power_scripts
>>>
>>> Changes taken from TRITON Errata27 workaround patch by Nishanth Menon.
>>>
>>> Signed-off-by: Lesly A M
>>> Cc: Nishanth Menon
>>> Cc: David Derrick
>>> Cc: Samuel Ortiz
>>> ---
>>> arch/arm/mach-omap2/omap_twl.c | 35 +
>>> arch/arm/mach-omap2/twl4030.c | 115
>>> +
>>> arch/arm/plat-omap/include/plat/voltage.h | 2 +
>>> drivers/mfd/twl4030-power.c | 74 ++
>>> include/linux/i2c/twl.h | 2 +
>>> 5 files changed, 228 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_twl.c b/arch/arm/mach-omap2/omap_twl.c
>>> index f0feab9..d6984b8 100644
>>> --- a/arch/arm/mach-omap2/omap_twl.c
>>> +++ b/arch/arm/mach-omap2/omap_twl.c
>>> @@ -176,6 +176,14 @@
>>> #define OMAP3_CLKSETUP_RET 31 /* 30.5 uS */
>>> #define OMAP3_CLKSETUP_OFF 1 /* 10 mS */
>>>
>>> +/*
>>> + * The clk/volt setuptime is adjusted to do the VDD1/VDD2 voltage rampup
>>> + * only after HFCLKIN is stabilized and the HFCLKOUT is enabled
>>> + */
>>> +#define CLKSETUP_TWL5030_ERRATA27 11567 /* 11.567 mS */
>>> +#define VOLTOFFSET_TWL5030_ERRATA27 516 /* 516 uS */
>>> +#define VOLTSETUP2_TWL5030_ERRATA27 11079 /* 11.079 mS */
>>> +
>>> static bool is_offset_valid;
>>> static u8 smps_offset;
>>>
>>> @@ -482,6 +490,33 @@ static struct omap_volt_pmic_info omap4_core_volt_info
>>> = {
>>> .uv_to_vsel = twl6030_uv_to_vsel,
>>> };
>>>
>>> +/**
>>> + * omap3_twl5030_errata27() - update the clk & volt setup time.
>>> + *
>>> + * Api to update the clk & volt setup time for TWL5030 errata 27.
>>> + */
>>> +void omap3_twl5030_errata27(void)
>>> +{
>>> + if (!cpu_is_omap34xx())
>>> + return;
>>> +
>>> + if (cpu_is_omap3630()) {
>>> + omap3630_mpu_volt_info.voltsetup_off.voltsetup2 =
>>> + VOLTSETUP2_TWL5030_ERRATA27;
>>> + omap3630_mpu_volt_info.volts
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Mon, Feb 21, 2011 at 10:22 AM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Feb 21, 2011 at 10:18:56AM +0200, Hiroshi DOYU wrote:
>> From: David Cohen
>> Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault
>> handling
>> Date: Wed, 16 Feb 2011 21:35:51 +0200
>>
>> > Add support to register an isr for IOMMU fault situations and adapt it
>> > to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
>> > module might want to be informed when errors happen in order to debug it
>> > or react.
>> >
>> > Signed-off-by: David Cohen
>> > ---
>> > arch/arm/mach-omap2/iommu2.c | 17 +-
>> > arch/arm/plat-omap/include/plat/iommu.h | 14 -
>> > arch/arm/plat-omap/iommu.c | 52
>> > ++-
>> > 3 files changed, 65 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> > index 49a1e5e..adb083e 100644
>> > --- a/arch/arm/mach-omap2/iommu2.c
>> > +++ b/arch/arm/mach-omap2/iommu2.c
>> > @@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj,
>> > bool on)
>> > static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
>> > {
>> > u32 stat, da;
>> > + u32 errs = 0;
>> >
>> > stat = iommu_read_reg(obj, MMU_IRQSTATUS);
>> > stat &= MMU_IRQ_MASK;
>> > - if (!stat)
>> > + if (!stat) {
>> > + *ra = 0;
>> > return 0;
>> > + }
>> >
>> > da = iommu_read_reg(obj, MMU_FAULT_AD);
>> > *ra = da;
>> >
>> > + 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_write_reg(obj, stat, MMU_IRQSTATUS);
>> >
>> > - return stat;
>> > + return errs;
>> > }
>> >
>> > static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
>> > diff --git a/arch/arm/plat-omap/include/plat/iommu.h
>> > b/arch/arm/plat-omap/include/plat/iommu.h
>> > index 19cbb5e..174f1b9 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 *isr_priv;
>>
>> Ideally I'd like to avoid having "isr_priv" in iommu since it's not
>> used for iommu but client needs the place to pass its info to its
>> custom handler. Any better idea?
>
> I'm not sure if it makes sense as I don't know the mailbox block, but
> maybe moving to GENIRQ ? Then IRQ subsystem would take care of the
> "dev_id"
Not sure if it fits in this case. It's a different module (IOMMU user)
which needs to get a callback from IOMMU when a fault happens.
Is there any GENIRQ usage currently in this scenario?
Br,
David
>
> --
> balbi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Mon, Feb 21, 2011 at 10:18 AM, Hiroshi DOYU wrote:
> From: David Cohen
> Subject: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault
> handling
> Date: Wed, 16 Feb 2011 21:35:51 +0200
>
>> Add support to register an isr for IOMMU fault situations and adapt it
>> to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
>> module might want to be informed when errors happen in order to debug it
>> or react.
>>
>> Signed-off-by: David Cohen
>> ---
>> arch/arm/mach-omap2/iommu2.c | 17 +-
>> arch/arm/plat-omap/include/plat/iommu.h | 14 -
>> arch/arm/plat-omap/iommu.c | 52
>> ++-
>> 3 files changed, 65 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> index 49a1e5e..adb083e 100644
>> --- a/arch/arm/mach-omap2/iommu2.c
>> +++ b/arch/arm/mach-omap2/iommu2.c
>> @@ -146,18 +146,31 @@ static void omap2_iommu_set_twl(struct iommu *obj,
>> bool on)
>> static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
>> {
>> u32 stat, da;
>> + u32 errs = 0;
>>
>> stat = iommu_read_reg(obj, MMU_IRQSTATUS);
>> stat &= MMU_IRQ_MASK;
>> - if (!stat)
>> + if (!stat) {
>> + *ra = 0;
>> return 0;
>> + }
>>
>> da = iommu_read_reg(obj, MMU_FAULT_AD);
>> *ra = da;
>>
>> + 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_write_reg(obj, stat, MMU_IRQSTATUS);
>>
>> - return stat;
>> + return errs;
>> }
>>
>> static void omap2_tlb_read_cr(struct iommu *obj, struct cr_regs *cr)
>> diff --git a/arch/arm/plat-omap/include/plat/iommu.h
>> b/arch/arm/plat-omap/include/plat/iommu.h
>> index 19cbb5e..174f1b9 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 *isr_priv;
>
> Ideally I'd like to avoid having "isr_priv" in iommu since it's not
> used for iommu but client needs the place to pass its info to its
> custom handler. Any better idea?
(*isr)() relies in the same situation, as it belongs to the client.
Without this priv_data, it's necessary to create a global variable to
store client's private data on client side. IMO, it is worse.
Br,
David
>
>
>> unsigned int refcount;
>> struct mutex iommu_lock; /* global for this whole object */
>> @@ -47,7 +48,7 @@ struct iommu {
>> struct list_head mmap;
>> struct mutex mmap_lock; /* protect mmap */
>>
>> - int (*isr)(struct iommu *obj);
>> + int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
>>
>> void *ctx; /* iommu context: registres saved area */
>> u32 da_start;
>> @@ -109,6 +110,13 @@ struct iommu_platform_data {
>> u32 da_end;
>> };
>>
>> +/* IOMMU errors */
>> +#define OMAP_IOMMU_ERR_TLB_MISS (1 << 0)
>> +#define OMAP_IOMMU_ERR_TRANS_FAULT (1 << 1)
>> +#define OMAP_IOMMU_ERR_EMU_MISS (1 << 2)
>> +#define OMAP_IOMMU_ERR_TBLWALK_FAULT (1 << 3)
>> +#define OMAP_IOMMU_ERR_MULTIHIT_FAULT (1 << 4)
>> +
>> #if defined(CONFIG_ARCH_OMAP1)
>> #error "iommu for this processor not implemented yet"
>> #else
>> @@ -161,6 +169,10 @@ extern size_t iopgtable_clear_entry(struct iommu *obj,
>> u32 iova);
>> extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end);
>> extern struct iommu *iommu_get(const char *name);
>> extern void iommu_put(struct iommu *obj);
>> +extern int iommu_set_isr(const char *name,
>> + int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
>> + void *priv),
>> + void *isr_priv);
>
[PATCH 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
Currently sched.h and wait.h have circular dependency between both.
wait.h defines macros wake_up*() which use macros TASK_* defined by
sched.h. But as sched.h indirectly includes wait.h, such wait.h header
file can't include sched.h too. The side effect is when some file
includes wait.h and tries to use its wake_up*() macros, it's necessary
to include sched.h also.
This patch moves all TASK_* macros from linux/sched.h to a new header
file linux/task_sched.h. This way, both sched.h and wait.h can include
task_sched.h and fix the circular dependency. No need to include sched.h
anymore when wake_up*() macros are used.
Signed-off-by: David Cohen
---
include/linux/sched.h | 61 +-
include/linux/task_sched.h | 64
include/linux/wait.h |1 +
3 files changed, 66 insertions(+), 60 deletions(-)
create mode 100644 include/linux/task_sched.h
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d747f94..c60dcee 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -90,6 +90,7 @@ struct sched_param {
#include
#include
#include
+#include
#include
@@ -169,63 +170,6 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq
*cfs_rq)
#endif
/*
- * Task state bitmask. NOTE! These bits are also
- * encoded in fs/proc/array.c: get_task_state().
- *
- * We have two separate sets of flags: task->state
- * is about runnability, while task->exit_state are
- * about the task exiting. Confusing, but this way
- * modifying one set can't modify the other one by
- * mistake.
- */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-#define __TASK_STOPPED 4
-#define __TASK_TRACED 8
-/* in tsk->exit_state */
-#define EXIT_ZOMBIE16
-#define EXIT_DEAD 32
-/* in tsk->state again */
-#define TASK_DEAD 64
-#define TASK_WAKEKILL 128
-#define TASK_WAKING256
-#define TASK_STATE_MAX 512
-
-#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW"
-
-extern char ___assert_task_state[1 - 2*!!(
- sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
-
-/* Convenience macros for the sake of set_task_state */
-#define TASK_KILLABLE (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
-#define TASK_STOPPED (TASK_WAKEKILL | __TASK_STOPPED)
-#define TASK_TRACED(TASK_WAKEKILL | __TASK_TRACED)
-
-/* Convenience macros for the sake of wake_up */
-#define TASK_NORMAL(TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE)
-#define TASK_ALL (TASK_NORMAL | __TASK_STOPPED | __TASK_TRACED)
-
-/* get_task_state() */
-#define TASK_REPORT(TASK_RUNNING | TASK_INTERRUPTIBLE | \
-TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-__TASK_TRACED)
-
-#define task_is_traced(task) ((task->state & __TASK_TRACED) != 0)
-#define task_is_stopped(task) ((task->state & __TASK_STOPPED) != 0)
-#define task_is_dead(task) ((task)->exit_state != 0)
-#define task_is_stopped_or_traced(task)\
- ((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
-#define task_contributes_to_load(task) \
- ((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
-(task->flags & PF_FREEZING) == 0)
-
-#define __set_task_state(tsk, state_value) \
- do { (tsk)->state = (state_value); } while (0)
-#define set_task_state(tsk, state_value) \
- set_mb((tsk)->state, (state_value))
-
-/*
* set_current_state() includes a barrier so that the write of current->state
* is correctly serialised wrt the caller's subsequent test of whether to
* actually sleep:
@@ -241,9 +185,6 @@ extern char ___assert_task_state[1 - 2*!!(
#define set_current_state(state_value) \
set_mb(current->state, (state_value))
-/* Task command name length */
-#define TASK_COMM_LEN 16
-
#include
/*
diff --git a/include/linux/task_sched.h b/include/linux/task_sched.h
new file mode 100644
index 000..3787387
--- /dev/null
+++ b/include/linux/task_sched.h
@@ -0,0 +1,64 @@
+#ifndef _LINUX_TASK_H
+#define _LINUX_TASK_H
+
+/*
+ * Task state bitmask. NOTE! These bits are also
+ * encoded in fs/proc/array.c: get_task_state().
+ *
+ * We have two separate sets of flags: task->state
+ * is about runnability, while task->exit_state are
+ * about the task exiting. Confusing, but this way
+ * modifying one set can't modify the other one by
+ * mistake.
+ */
+#define TASK_RUNNING 0
+#define TASK_INTERRUPTIBLE 1
+#define TASK_UNINTERRUPTIBLE 2
+#define __TASK_STOPPED 4
+#define __TASK_TRACED 8
+/* in tsk->exit_state */
+#define EXIT_ZOMBIE16
+#define EXIT_DEAD
[PATCH 0/1] Fix linux/wait.h header file
Hi, OMAP2 camera driver compilation is broken due to problems on linux/wait.h header file: drivers/media/video/omap24xxcam.c: In function 'omap24xxcam_vbq_complete': drivers/media/video/omap24xxcam.c:414: error: 'TASK_NORMAL' undeclared (first use in this function) drivers/media/video/omap24xxcam.c:414: error: (Each undeclared identifier is reported only once drivers/media/video/omap24xxcam.c:414: error: for each function it appears in.) make[3]: *** [drivers/media/video/omap24xxcam.o] Error 1 make[2]: *** [drivers/media/video] Error 2 make[1]: *** [drivers/media] Error 2 make: *** [drivers] Error 2 This file defines macros wake_up*() which use TASK_* defined on linux/sched.h. But sched.h cannot be included on wait.h due to a circular dependency between both files. This patch fixes such compilation and the circular dependency problem. Br, David --- David Cohen (1): headers: fix circular dependency between linux/sched.h and linux/wait.h include/linux/sched.h | 61 +- include/linux/task_sched.h | 64 include/linux/wait.h |1 + 3 files changed, 66 insertions(+), 60 deletions(-) create mode 100644 include/linux/task_sched.h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH resend] video: omap24xxcam: Fix compilation
On Mon, Feb 21, 2011 at 9:36 AM, Felipe Balbi wrote: > Hi, > > On Sat, Feb 19, 2011 at 06:04:58PM +0200, David Cohen wrote: >> > I have to disagree. The fundamental problem is the circular dependency >> > between those two files: >> > >> > sched.h uses wait_queue_head_t defined in wait.h >> > wait.h uses TASK_* defined in sched.h >> > >> > So, IMO the real fix would be clear out the circular dependency. Maybe >> > introducing to define those TASK_* symbols and include >> > that on sched.h and wait.h >> > >> > Just dig a quick and dirty to try it out and works like a charm >> >> We have 2 problems: >> - omap24xxcam compilation broken >> - circular dependency between sched.h and wait.h >> >> To fix the broken compilation we can do what the rest of the kernel is >> doing, which is to include sched.h. >> Then, the circular dependency is fixed by some different approach >> which would probably change *all* current usage of TASK_*. > > considering that 1 is caused by 2 I would fix 2. > >> IMO, there's no need to create a dependency between those issues. > > There's no dependency between them, it's just that the root cause for > this problem is a circular dependency between wait.h and sched.h I did a try to fix this circular dependency and the comment I got was to include sched.h in omap24xxcam.c file: http://marc.info/?l=linux-omap&m=129828637120270&w=2 I'm working to remove v4l2 internal device interface from omap24xxcam and then I need this driver's compilation fixed. The whole kernel is including sched.h when wake_up*() macro is used, so this should be our first solution IMO. As I said earlier, no need to make this compilation fix be dependent of wait.h fix (if it's really going to be changed). I think we should proceed with this patch. Br, David > > -- > balbi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 1:05 PM, Alexey Dobriyan wrote: > On Mon, Feb 21, 2011 at 12:20 PM, David Cohen wrote: >> Currently sched.h and wait.h have circular dependency between both. >> wait.h defines macros wake_up*() which use macros TASK_* defined by >> sched.h. But as sched.h indirectly includes wait.h, such wait.h header >> file can't include sched.h too. The side effect is when some file >> includes wait.h and tries to use its wake_up*() macros, it's necessary >> to include sched.h also. >> This patch moves all TASK_* macros from linux/sched.h to a new header >> file linux/task_sched.h. This way, both sched.h and wait.h can include >> task_sched.h and fix the circular dependency. No need to include sched.h >> anymore when wake_up*() macros are used. > > Just include in your driver. Sounds a reasonable solution for me. > This include splitting in small pieces is troublesome as well. But I disagree it's troublesome. It's transparent to everyone else. The only side effect is to not have to include sched.h when using a macro define on wait.h. > > Why are you moving TASK_COMM_LEN? This one can be moved back to sched.h. Br, David > >> include/linux/sched.h | 61 +- >> include/linux/task_sched.h | 64 >> >> include/linux/wait.h | 1 + >> 3 files changed, 66 insertions(+), 60 deletions(-) >> create mode 100644 include/linux/task_sched.h > >> --- a/include/linux/sched.h >> +++ b/include/linux/sched.h >> +#include > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 3:57 PM, Felipe Balbi wrote: > Hi, > > On Mon, Feb 21, 2011 at 03:51:25PM +0200, Alexey Dobriyan wrote: >> > I rather have the split done and kill the circular dependency. >> >> It's not circular for starters. > > how come ? wait.h depends on sched and sched.h depends on wait.h The tricky thing is wait.h doesn't depend on sched.h, but the file which uses wake_up*() macro defined on wait.h will depend on sched.h (what is still bad). wait.h should provide all dependencies to use a macro it defines. I'll send a new version for this patch following the comments I got. Let's see how it looks like. Br, David > > -- > balbi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/1] Fix linux/wait.h header file
Hi, OMAP2 camera driver compilation is broken due to problems on linux/wait.h header file: drivers/media/video/omap24xxcam.c: In function 'omap24xxcam_vbq_complete': drivers/media/video/omap24xxcam.c:414: error: 'TASK_NORMAL' undeclared (first use in this function) drivers/media/video/omap24xxcam.c:414: error: (Each undeclared identifier is reported only once drivers/media/video/omap24xxcam.c:414: error: for each function it appears in.) make[3]: *** [drivers/media/video/omap24xxcam.o] Error 1 make[2]: *** [drivers/media/video] Error 2 make[1]: *** [drivers/media] Error 2 make: *** [drivers] Error 2 This file defines macros wake_up*() which use TASK_* defined on linux/sched.h. But sched.h cannot be included on wait.h due to a circular dependency between both files. This patch fixes such compilation and the circular dependency problem. Br, David --- David Cohen (1): headers: fix circular dependency between linux/sched.h and linux/wait.h include/linux/sched.h | 58 +- include/linux/task_state.h | 61 include/linux/wait.h |1 + 3 files changed, 63 insertions(+), 57 deletions(-) create mode 100644 include/linux/task_state.h -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
Currently sched.h and wait.h have circular dependency between both.
wait.h defines macros wake_up*() which use macros TASK_* defined by
sched.h. But as sched.h indirectly includes wait.h, such wait.h header
file can't include sched.h too. The side effect is when some file
includes wait.h and tries to use its wake_up*() macros, it's necessary
to include sched.h also.
This patch moves all TASK_* macros from linux/sched.h to a new header
file linux/task_state.h. This way, both sched.h and wait.h can include
task_state.h and fix the circular dependency. No need to include sched.h
anymore when wake_up*() macros are used.
Signed-off-by: David Cohen
---
include/linux/sched.h | 58 +-
include/linux/task_state.h | 61
include/linux/wait.h |1 +
3 files changed, 63 insertions(+), 57 deletions(-)
create mode 100644 include/linux/task_state.h
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d747f94..a75b5ba 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -90,6 +90,7 @@ struct sched_param {
#include
#include
#include
+#include
#include
@@ -169,63 +170,6 @@ print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq
*cfs_rq)
#endif
/*
- * Task state bitmask. NOTE! These bits are also
- * encoded in fs/proc/array.c: get_task_state().
- *
- * We have two separate sets of flags: task->state
- * is about runnability, while task->exit_state are
- * about the task exiting. Confusing, but this way
- * modifying one set can't modify the other one by
- * mistake.
- */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-#define __TASK_STOPPED 4
-#define __TASK_TRACED 8
-/* in tsk->exit_state */
-#define EXIT_ZOMBIE16
-#define EXIT_DEAD 32
-/* in tsk->state again */
-#define TASK_DEAD 64
-#define TASK_WAKEKILL 128
-#define TASK_WAKING256
-#define TASK_STATE_MAX 512
-
-#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW"
-
-extern char ___assert_task_state[1 - 2*!!(
- sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
-
-/* Convenience macros for the sake of set_task_state */
-#define TASK_KILLABLE (TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
-#define TASK_STOPPED (TASK_WAKEKILL | __TASK_STOPPED)
-#define TASK_TRACED(TASK_WAKEKILL | __TASK_TRACED)
-
-/* Convenience macros for the sake of wake_up */
-#define TASK_NORMAL(TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE)
-#define TASK_ALL (TASK_NORMAL | __TASK_STOPPED | __TASK_TRACED)
-
-/* get_task_state() */
-#define TASK_REPORT(TASK_RUNNING | TASK_INTERRUPTIBLE | \
-TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-__TASK_TRACED)
-
-#define task_is_traced(task) ((task->state & __TASK_TRACED) != 0)
-#define task_is_stopped(task) ((task->state & __TASK_STOPPED) != 0)
-#define task_is_dead(task) ((task)->exit_state != 0)
-#define task_is_stopped_or_traced(task)\
- ((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
-#define task_contributes_to_load(task) \
- ((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
-(task->flags & PF_FREEZING) == 0)
-
-#define __set_task_state(tsk, state_value) \
- do { (tsk)->state = (state_value); } while (0)
-#define set_task_state(tsk, state_value) \
- set_mb((tsk)->state, (state_value))
-
-/*
* set_current_state() includes a barrier so that the write of current->state
* is correctly serialised wrt the caller's subsequent test of whether to
* actually sleep:
diff --git a/include/linux/task_state.h b/include/linux/task_state.h
new file mode 100644
index 000..36a8db8
--- /dev/null
+++ b/include/linux/task_state.h
@@ -0,0 +1,61 @@
+#ifndef _LINUX_TASK_H
+#define _LINUX_TASK_H
+
+/*
+ * Task state bitmask. NOTE! These bits are also
+ * encoded in fs/proc/array.c: get_task_state().
+ *
+ * We have two separate sets of flags: task->state
+ * is about runnability, while task->exit_state are
+ * about the task exiting. Confusing, but this way
+ * modifying one set can't modify the other one by
+ * mistake.
+ */
+#define TASK_RUNNING 0
+#define TASK_INTERRUPTIBLE 1
+#define TASK_UNINTERRUPTIBLE 2
+#define __TASK_STOPPED 4
+#define __TASK_TRACED 8
+/* in tsk->exit_state */
+#define EXIT_ZOMBIE16
+#define EXIT_DEAD 32
+/* in tsk->state again */
+#define TASK_DEAD 64
+#define TASK_WAKEKILL 128
+#define TASK_WAKING256
+#define TASK_STATE_MAX 512
+
+#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW&
Re: [PATCH v2 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 5:54 PM, Peter Zijlstra wrote: > On Mon, 2011-02-21 at 16:38 +0200, David Cohen wrote: >> Currently sched.h and wait.h have circular dependency between both. >> wait.h defines macros wake_up*() which use macros TASK_* defined by >> sched.h. But as sched.h indirectly includes wait.h, such wait.h header >> file can't include sched.h too. The side effect is when some file >> includes wait.h and tries to use its wake_up*() macros, it's necessary >> to include sched.h also. >> This patch moves all TASK_* macros from linux/sched.h to a new header >> file linux/task_state.h. This way, both sched.h and wait.h can include >> task_state.h and fix the circular dependency. No need to include sched.h >> anymore when wake_up*() macros are used. > > I think Alexey already told you what you done wrong. > > Also, I really don't like the task_state.h header, it assumes a lot of > things it doesn't include itself and only works because its using macros > and not inlines at it probably should. Like wait.h I'd say. The main issue is wait.h uses TASK_* macros but cannot properly include sched.h as it would create a circular dependency. So a file including wait.h is able to compile because the dependency of sched.h relies on wake_up*() macros and it's not always used. We can still drop everything else from task_state.h but the TASK_* macros and then the problem you just pointed out won't exist anymore. What do you think about it? Br, David > > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Mon, Feb 21, 2011 at 8:43 PM, Ramirez Luna, Omar wrote:
> Hi,
Hi,
Thanks for the comments.
>
> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen wrote:
>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> index 49a1e5e..adb083e 100644
>> --- a/arch/arm/mach-omap2/iommu2.c
>> +++ b/arch/arm/mach-omap2/iommu2.c
> ...
>>
>> da = iommu_read_reg(obj, MMU_FAULT_AD);
>> *ra = da;
>>
>> + 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;
>
> I still don't think this adds any value, "generic layer" and omap
> errors are the same thing in this case... OTOH OMAP 1710 (not
> supported by iommu yet) has the following bits:
>
> 3 Prefetch_err
> 2 Perm_fault
> 1 Tlb_miss
> 0 Trans_fault
>
> They don't match any of your "generic layer errors" masks for reading,
Have you noticed:
0 = OMAP_IOMMU_ERR_TRANS_FAULT
1 = OMAP_IOMMU_ERR_TLB_MISS
2 and 3 could be added.
> hence more generic errors will need to be defined, and then more OMAP#
> masks... I think we just need to stick with the mach specific errors,
> and let mach code handle its specifics when reporting.
How many are we talking about? I don't think every new OMAP version it
would completely re-invent IOMMU faults.
Generic errors codes make easier to threat possible IOMMU users which
have (partially or totally) common drivers for different OMAP
versions.
Unless it's really an out-of-control number of generic faults, I don't
see it as a real problem.
>
> But anyway it is just me...
>
>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
>> index f55f458..b0e0efc 100644
>> --- a/arch/arm/plat-omap/iommu.c
>> +++ b/arch/arm/plat-omap/iommu.c
>> @@ -780,25 +780,19 @@ static void iopgtable_clear_entry_all(struct iommu
>> *obj)
>> */
>> static irqreturn_t iommu_fault_handler(int irq, void *data)
>> {
>> - u32 stat, da;
>> + u32 da, errs;
>> u32 *iopgd, *iopte;
>> - int err = -EIO;
>> struct iommu *obj = 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);
>> + errs = iommu_report_fault(obj, &da);
>> clk_disable(obj->clk);
>> - if (!stat)
>> +
>> + /* Fault callback or TLB/PTE Dynamic loading */
>> + if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv))
>> return IRQ_HANDLED;
>
> BTW, why not changing the name isr for cb, it is confusing since there
> is another fault_isr called by mmu, AFAIK nobody uses obj->isr
The main purpose of this function is to be an ISR, not only callback.
But as you noticed, nobody is using it yet, but OMAP3 ISP should start
to use it soon.
>
>> iommu_disable(obj);
>> @@ -806,15 +800,16 @@ static irqreturn_t iommu_fault_handler(int irq, void
>> *data)
>> iopgd = iopgd_offset(obj, da);
>>
>> if (!iopgd_is_table(*iopgd)) {
>> - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n",
>> obj->name,
>> - da, iopgd, *iopgd);
>> + dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p "
>> + "*pgd:px%08x\n", obj->name, errs, da, iopgd, *iopgd);
>> return IRQ_NONE;
>> }
>>
>> iopte = iopte_offset(iopgd, da);
>>
>> - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
>> - obj->name, da, iopgd, *iopgd, iopte, *iopte);
>> + dev_err(obj->dev, "%s: errs:0x%08x da:0x%08x pgd:0x%p *pgd:0x%08x "
>> + "pte:0x%p *pte:0x%08x\n", obj->name, errs, da, iopgd, *iopgd,
>> + iopte, *iopte);
>>
>> return IRQ_NONE;
>
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Tue, Feb 22, 2011 at 1:25 AM, Ramirez Luna, Omar wrote:
> Hi,
>
> On Mon, Feb 21, 2011 at 3:12 PM, David Cohen wrote:
>> Generic errors codes make easier to threat possible IOMMU users which
>> have (partially or totally) common drivers for different OMAP
>> versions.
>
> Agree then.
Good we agreed on that.
>
>>>> + /* Fault callback or TLB/PTE Dynamic loading */
>>>> + if (obj->isr && !obj->isr(obj, da, errs, obj->isr_priv))
>>>> return IRQ_HANDLED;
>>>
>>> BTW, why not changing the name isr for cb, it is confusing since there
>>> is another fault_isr called by mmu, AFAIK nobody uses obj->isr
>>
>> The main purpose of this function is to be an ISR, not only callback.
>
> Yep, I just thought it was a bit too much of:
> (1) The real isr: iommu_fault_handler, set on request_irq
> (2) A plugged fault_isr for mach specific code: omap2_iommu_fault_isr,
> for error reporting.
> (3) And then a plugged custom isr, to be set by the user.
>
> Feel free to ignore.
(1) does nothing but to call (2) and then (3) if it exists and print
error message.
(2) as you said, it's mach specific and necessary for the generic upper layer.
(3) does what the IOMMU user wants and may replace (1) if it returns 0.
They belong to different layers and should coexist.
>
>>> So I think the following will be bad practice:
>>>
>>> mmu = iommu_get("iva2");
>>> if (!IS_ERR(mmu))
>>> mmu->isr = mmu_fault_callback;
>>>
>>> Shall we think anything to prevent such mis-usage?
>>
>> Well, the IOMMU user has access to IOMMU obj, so it can not only
>> change the (*isr)() but to mess with a lot of other stuff. The only
>> way to prevent it is to avoid user to have obj. But then, this fix (or
>> issue) does not belong to this patch.
>
> Agree, just pointing out.
That's a valid point, but it requires an intrusive approach to prevent
such mis-usage. For now we must trust the user won't mess with obj.
Br,
David
>
> Regards,
>
> Omar
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] headers: fix circular dependency between linux/sched.h and linux/wait.h
On Mon, Feb 21, 2011 at 7:27 PM, Oleg Nesterov wrote: > On 02/21, Oleg Nesterov wrote: >> >> On 02/21, Peter Zijlstra wrote: >> > >> > afaict its needed because struct signal_struct and struct sighand_struct >> > include a wait_queue_head_t. The inclusion seems to come through >> > completion.h, but afaict we don't actually need to include completion.h >> > because all we have is a pointer to a completion, which is perfectly >> > fine with an incomplete type. >> > >> > This all would suggest we move the signal bits into their own header >> > (include/linux/signal.h already exists and seems inviting). >> >> Agreed, sched.h contatins a lot of garbage, including the signal bits. >> >> As for signal_struct in particular I am not really sure, it is just >> misnamed. It is in fact "struct process" or "struct thread_group". But >> dequeue_signal/etc should go into signal.h. >> >> The only problem, it is not clear how to test such a change. > > Ah. sched.h includes signal.h, the testing is not the problem. If sched.h includes signal.h and we move wait_queue_head_t users to signal.h, it means signal.h should include wait.h and then it is a problem to include sched.h in wait.h. > > So, we can (at least) safely move some declarations. Safely, yes, but it won't solve the issue for TASK_* in wait.h. Br, David > > Oleg. > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
wrote:
> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen wrote:
>> Add support to register an isr for IOMMU fault situations and adapt it
>> to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
>> module might want to be informed when errors happen in order to debug it
>> or react.
>>
>> Signed-off-by: David Cohen
>> ---
>> arch/arm/mach-omap2/iommu2.c | 17 +-
>> arch/arm/plat-omap/include/plat/iommu.h | 14 -
>> arch/arm/plat-omap/iommu.c | 52
>> ++-
>> 3 files changed, 65 insertions(+), 18 deletions(-)
>>
>
>
>> @@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
>> }
>> EXPORT_SYMBOL_GPL(iommu_put);
>>
>> +int iommu_set_isr(const char *name,
>> + int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
>> + void *priv),
>> + void *isr_priv)
>> +{
>> + struct device *dev;
>> + struct iommu *obj;
>> +
>
> if the driver support multiple user for the same iommu why can only
> one callback be registered? should it support register multiple
> callback function (one per user)?
Can you define a scenario for that?
On OMAP3 ISP the multiple users are the multiple ISP submodule, but I
don't think it's necessary all submodule to have a specific callback.
ISP core layer should handle.
Br,
David
>
> Regards,
> Fernando.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
wrote:
> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen wrote:
>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>> wrote:
>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen wrote:
>>>> Add support to register an isr for IOMMU fault situations and adapt it
>>>> to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
>>>> module might want to be informed when errors happen in order to debug it
>>>> or react.
>>>>
>>>> Signed-off-by: David Cohen
>>>> ---
>>>> arch/arm/mach-omap2/iommu2.c | 17 +-
>>>> arch/arm/plat-omap/include/plat/iommu.h | 14 -
>>>> arch/arm/plat-omap/iommu.c | 52
>>>> ++-
>>>> 3 files changed, 65 insertions(+), 18 deletions(-)
>>>>
>>>
>>>
>>>> @@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
>>>> }
>>>> EXPORT_SYMBOL_GPL(iommu_put);
>>>>
>>>> +int iommu_set_isr(const char *name,
>>>> + int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
>>>> + void *priv),
>>>> + void *isr_priv)
>>>> +{
>>>> + struct device *dev;
>>>> + struct iommu *obj;
>>>> +
>>>
>>> if the driver support multiple user for the same iommu why can only
>>> one callback be registered? should it support register multiple
>>> callback function (one per user)?
>>
>> Can you define a scenario for that?
>> On OMAP3 ISP the multiple users are the multiple ISP submodule, but I
>> don't think it's necessary all submodule to have a specific callback.
>> ISP core layer should handle.
>
> Hi,
>
> In OMAP4 the cortex M3 is a double core processor and as each core is
> running they own version of the RTOS we threat them independently. So
> our driver which controls the remote processor sees two processor but
> both use the same iommu hw. When a iommu fault happens, at this
> moment, it is consider as a faltal error and it is no managed to
> recover and continue, instead a restart of the processor is needed, if
> the fault happens in core0 we need to reset core1 too and vice versa.
> if the iommu would support several user callbacks, we can register the
> callback which resets core0 and also the callback which resets core1
> and treat them as totally independent processors. Also we have an
> error event notifier driver, which is only in charge of notifying
> error events to userspace, so we would have multiple callbacks we
> could do this
I understood your point. In this case, I may not disagree about having
more than one callback per obj, although it doesn't seem a nice
scenario.
We can have a list of callbacks and call the entire list when a fault
happens. But it's necessary to pay attention it will happen in atomic
context and users should not abuse and register many callbacks. The
callback should *NOT* print useless messages and must verify the error
code to not execute useless steps.
In this context, callback and ISR cannot share a same pointer anymore.
>
> iommu < register fault callback for error notify driver
>
> instead of
>
> iommu <--- register fault callback for remote processor driver
> <register fault event for error notify driver.
>
> with that, we remove one dependency of the errornotify driver.
I don't know very well the errornotify driver, but to bypass it seems
be a good approach for me.
>
> Moreover, the iommu code support serveral users of the same hw iommu,
> and it does not make sense for me, that you can register only one
> callback, or if other user register its callback the previous one will
> be overwritten.
It's quite complex and dangerous this situation, as one driver can
crash another one. But I don't think drivers have much choice.
Hiroshi, do you have any different opinion for this subject? I can
send a v4 version for this patch giving support for multiple callbacks
per obj.
Br,
David
>
> Regards,
> Fernando.
>
>>
>> Br,
>>
>> David
>>
>>>
>>> Regards,
>>> Fernando.
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Wed, Feb 23, 2011 at 10:09 PM, Sakari Ailus wrote: > Guzman Lugo, Fernando wrote: >> Hi, > > Hi Fernando, > >> In OMAP4 the cortex M3 is a double core processor and as each core is >> running they own version of the RTOS we threat them independently. So >> our driver which controls the remote processor sees two processor but >> both use the same iommu hw. When a iommu fault happens, at this >> moment, it is consider as a faltal error and it is no managed to >> recover and continue, instead a restart of the processor is needed, if >> the fault happens in core0 we need to reset core1 too and vice versa. >> if the iommu would support several user callbacks, we can register the >> callback which resets core0 and also the callback which resets core1 >> and treat them as totally independent processors. Also we have an >> error event notifier driver, which is only in charge of notifying >> error events to userspace, so we would have multiple callbacks we >> could do this > > The original purpose of the patch, as far as I understand, is to allow > getting useful information for debugging purposes should an iommu fault > happen. > > Also, I'm not sure it's necessarily a good idea to just go and reset > the M3 cores in case an iommu fault happens --- this is very probably a > grave bug in the software running on those M3s. It should be fixed > instead of just hiding it. There will be consequences to host side as > well, won't there? That's really a good point. callbacks in this situation are mostly to debug purpose. Drivers shouldn't rely on that to work properly. David > >> iommu < register fault callback for error notify driver >> >> instead of >> >> iommu <--- register fault callback for remote processor driver >>> >> with that, we remove one dependency of the errornotify driver. > > I suppose this is not in mainline? > > Regards, > > -- > Sakari Ailus > sakari.ai...@maxwell.research.nokia.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Wed, Feb 23, 2011 at 11:48 PM, Guzman Lugo, Fernando
wrote:
> On Wed, Feb 23, 2011 at 2:56 PM, Sakari Ailus
> wrote:
>> David Cohen wrote:
>>> On Wed, Feb 23, 2011 at 3:39 PM, Guzman Lugo, Fernando
>>> wrote:
>>>> On Wed, Feb 23, 2011 at 3:45 AM, David Cohen wrote:
>>>>> On Wed, Feb 23, 2011 at 3:17 AM, Guzman Lugo, Fernando
>>>>> wrote:
>>>>>> On Wed, Feb 16, 2011 at 1:35 PM, David Cohen wrote:
>>>>>>> Add support to register an isr for IOMMU fault situations and adapt it
>>>>>>> to allow such (*isr)() to be used as fault callback. Drivers using IOMMU
>>>>>>> module might want to be informed when errors happen in order to debug it
>>>>>>> or react.
>>>>>>>
>>>>>>> Signed-off-by: David Cohen
>>>>>>> ---
>>>>>>> arch/arm/mach-omap2/iommu2.c | 17 +-
>>>>>>> arch/arm/plat-omap/include/plat/iommu.h | 14 -
>>>>>>> arch/arm/plat-omap/iommu.c | 52
>>>>>>> ++-
>>>>>>> 3 files changed, 65 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>
>>>>>>
>>>>>>> @@ -917,6 +912,33 @@ void iommu_put(struct iommu *obj)
>>>>>>> }
>>>>>>> EXPORT_SYMBOL_GPL(iommu_put);
>>>>>>>
>>>>>>> +int iommu_set_isr(const char *name,
>>>>>>> + int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
>>>>>>> + void *priv),
>>>>>>> + void *isr_priv)
>>>>>>> +{
>>>>>>> + struct device *dev;
>>>>>>> + struct iommu *obj;
>>>>>>> +
>>>>>>
>>>>>> if the driver support multiple user for the same iommu why can only
>>>>>> one callback be registered? should it support register multiple
>>>>>> callback function (one per user)?
>>>>>
>>>>> Can you define a scenario for that?
>>>>> On OMAP3 ISP the multiple users are the multiple ISP submodule, but I
>>>>> don't think it's necessary all submodule to have a specific callback.
>>>>> ISP core layer should handle.
>>>>
>>>> Hi,
>>>>
>>>> In OMAP4 the cortex M3 is a double core processor and as each core is
>>>> running they own version of the RTOS we threat them independently. So
>>>> our driver which controls the remote processor sees two processor but
>>>> both use the same iommu hw. When a iommu fault happens, at this
>>>> moment, it is consider as a faltal error and it is no managed to
>>>> recover and continue, instead a restart of the processor is needed, if
>>>> the fault happens in core0 we need to reset core1 too and vice versa.
>>>> if the iommu would support several user callbacks, we can register the
>>>> callback which resets core0 and also the callback which resets core1
>>>> and treat them as totally independent processors. Also we have an
>>>> error event notifier driver, which is only in charge of notifying
>>>> error events to userspace, so we would have multiple callbacks we
>>>> could do this
>>>
>>> I understood your point. In this case, I may not disagree about having
>>> more than one callback per obj, although it doesn't seem a nice
>>> scenario.
>>> We can have a list of callbacks and call the entire list when a fault
>>> happens. But it's necessary to pay attention it will happen in atomic
>>> context and users should not abuse and register many callbacks. The
>>> callback should *NOT* print useless messages and must verify the error
>>> code to not execute useless steps.
>>> In this context, callback and ISR cannot share a same pointer anymore.
>>
>> I think this is outside of the scope of the patch but...
>
> yes, the same behaviour was before the patches, but as the patches are
> changing the isr, I think it is a good time to modify, not in patch 2,
> but in a new patch to be added to the serie between patch 1 and 2, so
> that we dont need to change ISR part again after this set of patches.
Let's wait for Hiroshi's opinion and decide if I change or not the patches.
Br,
David
>
>>
>> To efficiently debug iommu faults (with a driver using iommu page
>> walking), besides the actual fault address the list of existing mappings
>> and the information which driver created them and for which purpose is
>> useful.
>>
>> The list of mappings is already available in the iommu structure. It'd
>> be nice if there was a function a driver could call to print them.
>
>>
>> I can only think of ugly ways to implement the other.
>>
>> Just my 5 cents (as we have no 2 cent coins here).
>>
>> Regards,
>>
>> --
>> Sakari Ailus
>> sakari.ai...@maxwell.research.nokia.com
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] OMAP: IOMMU: add support to callback during fault handling
On Thu, Feb 24, 2011 at 10:35 AM, Felipe Balbi wrote: > Hi, > > On Wed, Feb 23, 2011 at 10:09:05PM +0200, Sakari Ailus wrote: >> > In OMAP4 the cortex M3 is a double core processor and as each core is >> > running they own version of the RTOS we threat them independently. So >> > our driver which controls the remote processor sees two processor but >> > both use the same iommu hw. When a iommu fault happens, at this >> > moment, it is consider as a faltal error and it is no managed to >> > recover and continue, instead a restart of the processor is needed, if >> > the fault happens in core0 we need to reset core1 too and vice versa. >> > if the iommu would support several user callbacks, we can register the >> > callback which resets core0 and also the callback which resets core1 >> > and treat them as totally independent processors. Also we have an >> > error event notifier driver, which is only in charge of notifying >> > error events to userspace, so we would have multiple callbacks we >> > could do this >> >> The original purpose of the patch, as far as I understand, is to allow >> getting useful information for debugging purposes should an iommu fault >> happen. >> >> Also, I'm not sure it's necessarily a good idea to just go and reset >> the M3 cores in case an iommu fault happens --- this is very probably a >> grave bug in the software running on those M3s. It should be fixed >> instead of just hiding it. There will be consequences to host side as > > I have to agree here. Besides the fact that multiple callbacks is > outside the scope of this patch. This patch is already acked. What about leave it as it is and discuss multiple callbacks before release a new patch to support it? Br, David > > -- > balbi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH resend] video: omap24xxcam: Fix compilation
On Mon, Feb 21, 2011 at 2:21 PM, Felipe Balbi wrote: > On Mon, Feb 21, 2011 at 02:09:07PM +0200, David Cohen wrote: >> On Mon, Feb 21, 2011 at 9:36 AM, Felipe Balbi wrote: >> > Hi, >> > >> > On Sat, Feb 19, 2011 at 06:04:58PM +0200, David Cohen wrote: >> >> > I have to disagree. The fundamental problem is the circular dependency >> >> > between those two files: >> >> > >> >> > sched.h uses wait_queue_head_t defined in wait.h >> >> > wait.h uses TASK_* defined in sched.h >> >> > >> >> > So, IMO the real fix would be clear out the circular dependency. Maybe >> >> > introducing to define those TASK_* symbols and include >> >> > that on sched.h and wait.h >> >> > >> >> > Just dig a quick and dirty to try it out and works like a charm >> >> >> >> We have 2 problems: >> >> - omap24xxcam compilation broken >> >> - circular dependency between sched.h and wait.h >> >> >> >> To fix the broken compilation we can do what the rest of the kernel is >> >> doing, which is to include sched.h. >> >> Then, the circular dependency is fixed by some different approach >> >> which would probably change *all* current usage of TASK_*. >> > >> > considering that 1 is caused by 2 I would fix 2. >> > >> >> IMO, there's no need to create a dependency between those issues. >> > >> > There's no dependency between them, it's just that the root cause for >> > this problem is a circular dependency between wait.h and sched.h >> >> I did a try to fix this circular dependency and the comment I got was >> to include sched.h in omap24xxcam.c file: >> http://marc.info/?l=linux-omap&m=129828637120270&w=2 >> >> I'm working to remove v4l2 internal device interface from omap24xxcam >> and then I need this driver's compilation fixed. >> The whole kernel is including sched.h when wake_up*() macro is used, >> so this should be our first solution IMO. >> As I said earlier, no need to make this compilation fix be dependent >> of wait.h fix (if it's really going to be changed). >> >> I think we should proceed with this patch. > > I would wait to hear from Ingo or Peter who are the maintainers for that > part, but fine by me. How about to proceed with this patch? Regards, David > > -- > balbi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap:iommu-added cache flushing operation for L2 cache
Hi,
On Tue, Mar 1, 2011 at 9:46 PM, Fernando Guzman Lugo
wrote:
> From: Ramesh Gupta
No patch body description at all?
Can we get at least something here?
Regards,
David
>
> Signed-off-by: Ramesh Gupta
> Signed-off-by: Hari Kanigeri
> ---
> arch/arm/plat-omap/iommu.c | 22 --
> 1 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index e3eb038..aeb2c33 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -471,22 +471,15 @@ EXPORT_SYMBOL_GPL(foreach_iommu_device);
> */
> static void flush_iopgd_range(u32 *first, u32 *last)
> {
> - /* FIXME: L2 cache should be taken care of if it exists */
> - do {
> - asm("mcr p15, 0, %0, c7, c10, 1 @ flush_pgd"
> - : : "r" (first));
> - first += L1_CACHE_BYTES / sizeof(*first);
> - } while (first <= last);
> + dmac_flush_range(first, last);
> + outer_flush_range(virt_to_phys(first), virt_to_phys(last));
> }
>
> +
> static void flush_iopte_range(u32 *first, u32 *last)
> {
> - /* FIXME: L2 cache should be taken care of if it exists */
> - do {
> - asm("mcr p15, 0, %0, c7, c10, 1 @ flush_pte"
> - : : "r" (first));
> - first += L1_CACHE_BYTES / sizeof(*first);
> - } while (first <= last);
> + dmac_flush_range(first, last);
> + outer_flush_range(virt_to_phys(first), virt_to_phys(last));
> }
>
> static void iopte_free(u32 *iopte)
> @@ -750,7 +743,7 @@ size_t iopgtable_clear_entry(struct iommu *obj, u32 da)
> }
> EXPORT_SYMBOL_GPL(iopgtable_clear_entry);
>
> -static void iopgtable_clear_entry_all(struct iommu *obj)
> +void iopgtable_clear_entry_all(struct iommu *obj)
> {
> int i;
>
> @@ -777,7 +770,7 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
>
> spin_unlock(&obj->page_table_lock);
> }
> -
> +EXPORT_SYMBOL_GPL(iopgtable_clear_entry_all);
> /*
> * Device IOMMU generic operations
> */
> @@ -1068,6 +1061,7 @@ static void iopte_cachep_ctor(void *iopte)
> clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
> }
>
> +
> static int __init omap_iommu_init(void)
> {
> struct kmem_cache *p;
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP:iommu - pgd and pte entries weren't getting flushed out
Hi,
On Tue, Mar 1, 2011 at 9:46 PM, Fernando Guzman Lugo
wrote:
> From: Hari Kanigeri
>
> pgd and pte entries weren't getting flushed out leading to MMU faults.
May I ask you to add to the patch body description why it's wrong and
why your solution is necessary?
Br,
David
>
> Signed-off-by: Hari Kanigeri
> ---
> arch/arm/plat-omap/iommu.c | 12 ++--
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index aeb2c33..e9473ff 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -508,7 +508,7 @@ static u32 *iopte_alloc(struct iommu *obj, u32 *iopgd,
> u32 da)
> return ERR_PTR(-ENOMEM);
>
> *iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
> - flush_iopgd_range(iopgd, iopgd);
> + flush_iopgd_range(iopgd, iopgd + 1);
>
> dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
> } else {
> @@ -537,7 +537,7 @@ static int iopgd_alloc_section(struct iommu *obj, u32 da,
> u32 pa, u32 prot)
> }
>
> *iopgd = (pa & IOSECTION_MASK) | prot | IOPGD_SECTION;
> - flush_iopgd_range(iopgd, iopgd);
> + flush_iopgd_range(iopgd, iopgd + 1);
> return 0;
> }
>
> @@ -554,7 +554,7 @@ static int iopgd_alloc_super(struct iommu *obj, u32 da,
> u32 pa, u32 prot)
>
> for (i = 0; i < 16; i++)
> *(iopgd + i) = (pa & IOSUPER_MASK) | prot | IOPGD_SUPER;
> - flush_iopgd_range(iopgd, iopgd + 15);
> + flush_iopgd_range(iopgd, iopgd + 16);
> return 0;
> }
>
> @@ -567,7 +567,7 @@ static int iopte_alloc_page(struct iommu *obj, u32 da,
> u32 pa, u32 prot)
> return PTR_ERR(iopte);
>
> *iopte = (pa & IOPAGE_MASK) | prot | IOPTE_SMALL;
> - flush_iopte_range(iopte, iopte);
> + flush_iopte_range(iopte, iopte + 1);
>
> dev_vdbg(obj->dev, "%s: da:%08x pa:%08x pte:%p *pte:%08x\n",
> __func__, da, pa, iopte, *iopte);
> @@ -592,7 +592,7 @@ static int iopte_alloc_large(struct iommu *obj, u32 da,
> u32 pa, u32 prot)
>
> for (i = 0; i < 16; i++)
> *(iopte + i) = (pa & IOLARGE_MASK) | prot | IOPTE_LARGE;
> - flush_iopte_range(iopte, iopte + 15);
> + flush_iopte_range(iopte, iopte + 16);
> return 0;
> }
>
> @@ -763,7 +763,7 @@ void iopgtable_clear_entry_all(struct iommu *obj)
> iopte_free(iopte_offset(iopgd, 0));
>
> *iopgd = 0;
> - flush_iopgd_range(iopgd, iopgd);
> + flush_iopgd_range(iopgd, iopgd + 1);
> }
>
> flush_iotlb_all(obj);
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: omap3isp cache error when unloading
Hi,
[snip]
> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
> based on 2.6.38-rc5. I didn't have the problem with 2.6.37, either.
> It's actually not related to mis-configuring the ISP pipeline like I
> thought at first- it also happens after I have successfully captured images.
>
> I've since tracked down the problem, although I don't understand the
> cache management well enough to be sure it's a proper fix, so hopefully
> some new recipients on this can make suggestions/comments.
>
> The patch below solves the problem, which modifies a commit by Fernando
> Guzman Lugo from December.
>
> -Michael
>
> From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
> From: Michael Jones
> Date: Thu, 3 Mar 2011 16:50:39 +0100
> Subject: [PATCH] fix iovmm slab cache error on module unload
>
> modify "OMAP: iommu: create new api to set valid da range"
>
> This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
> ---
> arch/arm/plat-omap/iovmm.c | 5 -
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 6dc1296..2fba6f1 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
> *obj, u32 da,
> alignement = PAGE_SIZE;
>
> if (flags & IOVMF_DA_ANON) {
> - start = obj->da_start;
> + /*
> + * Reserve the first page for NULL
> + */
> + start = obj->da_start + PAGE_SIZE;
IMO if obj->da_start != 0, no need to add PAGE_SIZE. Otherwise, it
does make sense to correct wrong obj->da_start == 0. Another thing is
this piece of code is using alignement (alignment) variable instead of
PAGE_SIZE (which is the same value).
Br,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: omap3isp cache error when unloading
On Fri, Mar 4, 2011 at 4:39 PM, Michael Jones
wrote:
> Hi David,
Hi Michael,
>
> On 03/04/2011 02:12 PM, David Cohen wrote:
>> Hi,
>>
>> [snip]
>>
>>> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
>>> based on 2.6.38-rc5. I didn't have the problem with 2.6.37, either.
>>> It's actually not related to mis-configuring the ISP pipeline like I
>>> thought at first- it also happens after I have successfully captured images.
>>>
>>> I've since tracked down the problem, although I don't understand the
>>> cache management well enough to be sure it's a proper fix, so hopefully
>>> some new recipients on this can make suggestions/comments.
>>>
>>> The patch below solves the problem, which modifies a commit by Fernando
>>> Guzman Lugo from December.
>>>
>>> -Michael
>>>
>>> From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
>>> From: Michael Jones
>>> Date: Thu, 3 Mar 2011 16:50:39 +0100
>>> Subject: [PATCH] fix iovmm slab cache error on module unload
>>>
>>> modify "OMAP: iommu: create new api to set valid da range"
>>>
>>> This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
>>> ---
>>> arch/arm/plat-omap/iovmm.c | 5 -
>>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>> index 6dc1296..2fba6f1 100644
>>> --- a/arch/arm/plat-omap/iovmm.c
>>> +++ b/arch/arm/plat-omap/iovmm.c
>>> @@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct
>>> iommu *obj, u32 da,
>>> alignement = PAGE_SIZE;
>>>
>>> if (flags & IOVMF_DA_ANON) {
>>> - start = obj->da_start;
>>> + /*
>>> + * Reserve the first page for NULL
>>> + */
>>> + start = obj->da_start + PAGE_SIZE;
>>
>> IMO if obj->da_start != 0, no need to add PAGE_SIZE. Otherwise, it
>> does make sense to correct wrong obj->da_start == 0. Another thing is
>> this piece of code is using alignement (alignment) variable instead of
>> PAGE_SIZE (which is the same value).
>>
>> Br,
>>
>> David
>
> I believe the following patch addresses your comments. I couldn't
> resist also fixing the misspelling of alignment when I was using the
> variable in my patch.
>
> -Michael
>
> From 2712f2fd087ca782e964c912c7f1973e7d84f2b7 Mon Sep 17 00:00:00 2001
> From: Michael Jones
> Date: Fri, 4 Mar 2011 15:09:48 +0100
> Subject: [PATCH] omap: iovmm: disallow mapping NULL address
>
> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
> the NULL address if da_start==0, which would then not get unmapped.
> Disallow this again. And spell variable 'alignment' correctly.
>
> Signed-off-by: Michael Jones
> ---
> arch/arm/plat-omap/iovmm.c | 16 ++--
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 6dc1296..11c9b76 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
> *obj, u32 da,
> size_t bytes, u32 flags)
> {
> struct iovm_struct *new, *tmp;
> - u32 start, prev_end, alignement;
> + u32 start, prev_end, alignment;
>
> if (!obj || !bytes)
> return ERR_PTR(-EINVAL);
>
> start = da;
> - alignement = PAGE_SIZE;
> + alignment = PAGE_SIZE;
>
> if (flags & IOVMF_DA_ANON) {
> - start = obj->da_start;
> + /* Don't map address 0 */
> + if (obj->da_start)
> + start = obj->da_start;
> + else
> + start = obj->da_start + alignment;
It seems to be fine for me now. Let's see what Hiroshi says.
Regards,
David
>
> if (flags & IOVMF_LINEAR)
> - alignement = iopgsz_max(bytes);
> - start = roundup(start, alignement);
> + alignment = iopgsz_max(bytes);
> + start = roundup(start, alignment);
> } else if (start < obj->da_start || start > obj->da_end ||
> obj->da_end - start < bytes) {
>
Re: omap3isp cache error when unloading
[snip]
>> From 2712f2fd087ca782e964c912c7f1973e7d84f2b7 Mon Sep 17 00:00:00 2001
>> From: Michael Jones
>> Date: Fri, 4 Mar 2011 15:09:48 +0100
>> Subject: [PATCH] omap: iovmm: disallow mapping NULL address
>>
>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> the NULL address if da_start==0, which would then not get unmapped.
>> Disallow this again. And spell variable 'alignment' correctly.
>>
>> Signed-off-by: Michael Jones
>> ---
>> arch/arm/plat-omap/iovmm.c | 16 ++--
>> 1 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>> index 6dc1296..11c9b76 100644
>> --- a/arch/arm/plat-omap/iovmm.c
>> +++ b/arch/arm/plat-omap/iovmm.c
>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct
>> iommu *obj, u32 da,
>> size_t bytes, u32 flags)
>> {
>> struct iovm_struct *new, *tmp;
>> - u32 start, prev_end, alignement;
>> + u32 start, prev_end, alignment;
>>
>> if (!obj || !bytes)
>> return ERR_PTR(-EINVAL);
>>
>> start = da;
>> - alignement = PAGE_SIZE;
>> + alignment = PAGE_SIZE;
>>
>> if (flags & IOVMF_DA_ANON) {
>> - start = obj->da_start;
>> + /* Don't map address 0 */
>> + if (obj->da_start)
>> + start = obj->da_start;
>> + else
>> + start = obj->da_start + alignment;
>
> It seems to be fine for me now. Let's see what Hiroshi says.
Sorry, I'm afraid I changed my mind after take a look into the driver. :)
Try to correct obj->da_start in the functions iommu_set_da_range() and
omap_iommu_probe(). That should be the correct way. Your patch doesn't
fix this situation when IOVMF_DA_ANON isn't set.
After obj->da_start is correctly set, your current patch is non longer required.
Regards,
David
>
> Regards,
>
> David
>
>>
>> if (flags & IOVMF_LINEAR)
>> - alignement = iopgsz_max(bytes);
>> - start = roundup(start, alignement);
>> + alignment = iopgsz_max(bytes);
>> + start = roundup(start, alignment);
>> } else if (start < obj->da_start || start > obj->da_end ||
>> obj->da_end - start < bytes) {
>> return ERR_PTR(-EINVAL);
>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
>> *obj, u32 da,
>> goto found;
>>
>> if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>> - start = roundup(tmp->da_end + 1, alignement);
>> + start = roundup(tmp->da_end + 1, alignment);
>>
>> prev_end = tmp->da_end;
>> }
>> --
>> 1.7.4.1
>>
>>
>>
>> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
>> Registergericht: Amtsgericht Stuttgart, HRB 271090
>> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 05/19] OMAP3+: voltage: use IS_ERR_OR_NULL
Hi Nishanth,
On Sat, Mar 5, 2011 at 5:29 PM, Nishanth Menon wrote:
> Use IS_ERR_OR_NULL macro instead of !xyz || IS_ERR(xyz) usage.
>
> Signed-off-by: Nishanth Menon
> ---
> arch/arm/mach-omap2/voltage.c | 22 +++---
> 1 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 53c399f..76bcaee 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -682,7 +682,7 @@ unsigned long omap_voltage_get_nom_volt(struct
> voltagedomain *voltdm)
> {
> struct omap_vdd_info *vdd;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
I have one comment here. voltdm is received as parameter and it's
already checked by IS_ERR(). Is there any specific reason for that?
IS_ERR() doesn't suppose to be a macro to check if the pointer is
valid or not, but to know if there's an invalid value with error code
in the pointer value. It's useful when you have a function which
returns a pointer but it can return an error code when it fails.
Please, note that IS_ERR() won't detect invalid pointers which does
not represent an error code, so it's not correct to rely on it for
this purpose.
IMO, instead of change to IS_ERR_OR_NULL(), you could only remove
IS_ERR(). The same apply for the other cases.
Kind regards,
David Cohen
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return 0;
> }
> @@ -703,7 +703,7 @@ unsigned long omap_vp_get_curr_volt(struct voltagedomain
> *voltdm)
> struct omap_vdd_info *vdd;
> u8 curr_vsel;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return 0;
> }
> @@ -738,7 +738,7 @@ void omap_vp_enable(struct voltagedomain *voltdm)
> struct omap_vdd_info *vdd;
> u32 vpconfig;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return;
> }
> @@ -776,7 +776,7 @@ void omap_vp_disable(struct voltagedomain *voltdm)
> u32 vpconfig;
> int timeout;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return;
> }
> @@ -829,7 +829,7 @@ int omap_voltage_scale_vdd(struct voltagedomain *voltdm,
> {
> struct omap_vdd_info *vdd;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return -EINVAL;
> }
> @@ -858,7 +858,7 @@ void omap_voltage_reset(struct voltagedomain *voltdm)
> {
> unsigned long target_uvdc;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return;
> }
> @@ -890,7 +890,7 @@ void omap_voltage_get_volttable(struct voltagedomain
> *voltdm,
> {
> struct omap_vdd_info *vdd;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return;
> }
> @@ -921,7 +921,7 @@ struct omap_volt_data *omap_voltage_get_voltdata(struct
> voltagedomain *voltdm,
> struct omap_vdd_info *vdd;
> int i;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return ERR_PTR(-EINVAL);
> }
> @@ -959,7 +959,7 @@ int omap_voltage_register_pmic(struct voltagedomain
> *voltdm,
> {
> struct omap_vdd_info *vdd;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return -EINVAL;
> }
> @@ -986,7 +986,7 @@ struct dentry *omap_voltage_get_dbgdir(struct
> voltagedomain *voltdm)
> {
> struct omap_vdd_info *vdd;
>
> - if (!voltdm || IS_ERR(voltdm)) {
> + if (IS_ERR_OR_NULL(voltdm)) {
> pr_warning("%s: VDD specified does not exist!\n", __func__);
> return NULL;
> }
> @@ -1011,7 +1011,7 @@
Re: [PATCH] omap: iommu: disallow mapping NULL address
On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote: > On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones > wrote: >> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 >> From: Michael Jones >> Date: Mon, 7 Mar 2011 13:36:15 +0100 >> Subject: [PATCH] omap: iommu: disallow mapping NULL address >> >> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping >> the NULL address if da_start==0. Force da_start to exclude the >> first page. > > what about devices that uses page 0? ipu after reset always starts > from 0x how could we map that address?? from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? Br, David > > Regards, > Fernando. > >> >> Signed-off-by: Michael Jones >> --- >> arch/arm/plat-omap/iommu.c | 6 -- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c >> index 5990ea6..dcb5513 100644 >> --- a/arch/arm/plat-omap/iommu.c >> +++ b/arch/arm/plat-omap/iommu.c >> @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 >> end) >> if (end < start || !PAGE_ALIGN(start | end)) >> return -EINVAL; >> >> - obj->da_start = start; >> + obj->da_start = max(start, (u32)PAGE_SIZE); >> obj->da_end = end; >> >> return 0; >> @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct >> platform_device *pdev) >> obj->name = pdata->name; >> obj->dev = &pdev->dev; >> obj->ctx = (void *)obj + sizeof(*obj); >> - obj->da_start = pdata->da_start; >> + >> + /* reserve the first page for NULL */ >> + obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE); >> obj->da_end = pdata->da_end; >> >> mutex_init(&obj->iommu_lock); >> -- >> 1.7.4.1 >> >> >> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler >> Registergericht: Amtsgericht Stuttgart, HRB 271090 >> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap: iommu: disallow mapping NULL address
On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote: > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote: >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando >> wrote: >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones >>> wrote: >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 >>>> From: Michael Jones >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100 >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address >>>> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping >>>> the NULL address if da_start==0. Force da_start to exclude the >>>> first page. >>> >>> what about devices that uses page 0? ipu after reset always starts >>> from 0x how could we map that address?? >> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it? > > unlike DSP that you can load a register with the addres the DSP will > boot, IPU core always starts from address 0x, so if you take > IPU out of reset it will try to access address 0x0 if not map it, > there will be a mmu fault. Hm. Looks like the iommu should not restrict any da. The valid da range should rely only on pdata. Michael, what about just update ISP's da_start on omap-iommu.c file? Set it to 0x1000. Hiroshi, any opinion? Br, David > > Regards, > Fernando. > >> >> Br, >> >> David >> >>> >>> Regards, >>> Fernando. >>> >>>> >>>> Signed-off-by: Michael Jones >>>> --- >>>> arch/arm/plat-omap/iommu.c | 6 -- >>>> 1 files changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c >>>> index 5990ea6..dcb5513 100644 >>>> --- a/arch/arm/plat-omap/iommu.c >>>> +++ b/arch/arm/plat-omap/iommu.c >>>> @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, >>>> u32 end) >>>> if (end < start || !PAGE_ALIGN(start | end)) >>>> return -EINVAL; >>>> >>>> - obj->da_start = start; >>>> + obj->da_start = max(start, (u32)PAGE_SIZE); >>>> obj->da_end = end; >>>> >>>> return 0; >>>> @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct >>>> platform_device *pdev) >>>> obj->name = pdata->name; >>>> obj->dev = &pdev->dev; >>>> obj->ctx = (void *)obj + sizeof(*obj); >>>> - obj->da_start = pdata->da_start; >>>> + >>>> + /* reserve the first page for NULL */ >>>> + obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE); >>>> obj->da_end = pdata->da_end; >>>> >>>> mutex_init(&obj->iommu_lock); >>>> -- >>>> 1.7.4.1 >>>> >>>> >>>> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler >>>> Registergericht: Amtsgericht Stuttgart, HRB 271090 >>>> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap: iommu: disallow mapping NULL address
On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote: > Hi David, Hi Laurent, > > On Monday 07 March 2011 20:41:21 David Cohen wrote: >> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote: >> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote: >> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote: >> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote: >> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 >> >>>> From: Michael Jones >> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100 >> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address >> >>>> >> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping >> >>>> the NULL address if da_start==0. Force da_start to exclude the >> >>>> first page. >> >>> >> >>> what about devices that uses page 0? ipu after reset always starts >> >>> from 0x how could we map that address?? >> >> >> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you >> >> want it? >> > >> > unlike DSP that you can load a register with the addres the DSP will >> > boot, IPU core always starts from address 0x, so if you take >> > IPU out of reset it will try to access address 0x0 if not map it, >> > there will be a mmu fault. >> >> Hm. Looks like the iommu should not restrict any da. The valid da >> range should rely only on pdata. >> Michael, what about just update ISP's da_start on omap-iommu.c file? >> Set it to 0x1000. > > What about patching the OMAP3 ISP driver to use a non-zero value (maybe -1) as > an invalid/freed pointer ? I wouldn't be comfortable to use 0 (or NULL) value as valid address on ISP driver. The 'da' range (da_start and da_end) is defined per VM and specified as platform data. IMO, to set da_start = 0x1000 seems to be a correct approach for ISP as it's the only client for its IOMMU instance. Regards, David > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap: iommu: disallow mapping NULL address
Hi Fernando, On Tue, Mar 8, 2011 at 11:13 AM, Sakari Ailus wrote: > Guzman Lugo, Fernando wrote: >> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote: >>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando >>> wrote: >>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones >>>> wrote: >>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001 >>>>> From: Michael Jones >>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100 >>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address >>>>> >>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping >>>>> the NULL address if da_start==0. Force da_start to exclude the >>>>> first page. >>>> >>>> what about devices that uses page 0? ipu after reset always starts >>>> from 0x how could we map that address?? >>> >>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want >>> it? >> >> unlike DSP that you can load a register with the addres the DSP will >> boot, IPU core always starts from address 0x, so if you take >> IPU out of reset it will try to access address 0x0 if not map it, >> there will be a mmu fault. > > I think the driver for IPU (what is it, btw.?) must map the NULL address > explicitly. It cannot rely on automatic allocation of the NULL address > by the iommu even if it was the first allocation. That's an interesting question. My first thought was "it's not automatic allocation", because it seems you know the specific 'da' IPU needs. But then, looking into the driver's API, the automatic allocation is defined whether the argument da == 0 (automatic allocation) or da != 0 (fixed da). So, by default, the IOMMU driver does not see da == 0 as valid address for fixed da. Then, why only automatic allocation should use such address? My second point is: if you're using automatic allocation, you *cannot* rely on specific da to be used, as it would be up to IOMMU driver to choose. So, doesn't matter the option, your driver seems to be wrong, unless I'm missing something. If you were using fixed da passing da = 0, you were just being lucky that it was the first request and automatic allocation returned da == 0. IMO either first page is not allowed at all or OMAP's IOMMU API should change the way it checks if it's fixed da or not. Kind regards, David > > -- > Sakari Ailus > sakari.ai...@maxwell.research.nokia.com > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
IOVMM driver checks input 'da == 0' when mapping address to determine whether user wants fixed 'da' or not. At the same time, it doesn't disallow address 0x0 to be used, what creates an ambiguous situation. This patch set moves fixed 'da' check to the input flags. It also fixes da_start value for ISP IOMMU instance. David Cohen (2): omap3: change ISP's IOMMU da_start address omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags Michael Jones (1): omap: iovmm: disallow mapping NULL address arch/arm/mach-omap2/omap-iommu.c |2 +- arch/arm/plat-omap/iovmm.c | 28 ++-- 2 files changed, 19 insertions(+), 11 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] omap3: change ISP's IOMMU da_start address
ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.
Signed-off-by: David Cohen
---
arch/arm/mach-omap2/omap-iommu.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
.name = "isp",
.nr_tlb_entries = 8,
.clk_name = "cam_ick",
- .da_start = 0x0,
+ .da_start = 0x1000,
.da_end = 0xF000,
},
},
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] omap: iovmm: disallow mapping NULL address
From: Michael Jones
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0, which would then not get unmapped.
Disallow this again. And spell variable 'alignment' correctly.
Signed-off-by: Michael Jones
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..11c9b76 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags & IOVMF_DA_ANON) {
- start = obj->da_start;
+ /* Don't map address 0 */
+ if (obj->da_start)
+ start = obj->da_start;
+ else
+ start = obj->da_start + alignment;
if (flags & IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start < obj->da_start || start > obj->da_end ||
obj->da_end - start < bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
- start = roundup(tmp->da_end + 1, alignement);
+ start = roundup(tmp->da_end + 1, alignment);
prev_end = tmp->da_end;
}
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED
It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user. IOVMF_DA_ANON will be automatically set if
IOVMF_DA_FIXED isn't set.
Signed-off-by: David Cohen
---
arch/arm/plat-omap/iovmm.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 11c9b76..dde9cb0 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
sg_table *sgt,
flags &= IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags & IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -713,7 +714,8 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags &= IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags & IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
sgt = sgtable_alloc(bytes, flags, da, 0);
if (IS_ERR(sgt)) {
@@ -803,7 +805,8 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
bytes,
flags &= IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags & IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -862,7 +865,8 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags &= IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+ if (~flags & IOVMF_DA_FIXED)
+ flags |= IOVMF_DA_ANON;
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] omap3: change ISP's IOMMU da_start address
Hi Sakari,
On Tue, Mar 8, 2011 at 4:06 PM, Sakari Ailus
wrote:
> David Cohen wrote:
>> ISP doesn't consider 0x0 as a valid address, so it should explicitly
>> exclude first page from allowed 'da' range.
>>
>> Signed-off-by: David Cohen
>> ---
>> arch/arm/mach-omap2/omap-iommu.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap-iommu.c
>> b/arch/arm/mach-omap2/omap-iommu.c
>> index 3fc5dc7..3bea489 100644
>> --- a/arch/arm/mach-omap2/omap-iommu.c
>> +++ b/arch/arm/mach-omap2/omap-iommu.c
>> @@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
>> .name = "isp",
>> .nr_tlb_entries = 8,
>> .clk_name = "cam_ick",
>> - .da_start = 0x0,
>> + .da_start = 0x1000,
>
> The NULL address is still valid for the MMU. Can the IOVMF_DA_ANON
> mapping be specified by the API to be always non-NULL?
The patch 1/3 does that :)
>
> This way it would be possible to combine IOVMF_DA_FIXED and
> IOVMF_DA_ANON mappings in the same IOMMU while still being able to rely
> that IOVMF_DA_ANON mappings would always be non-NULL.
Indeed the IOMMU driver supports it. The flag is set by iovm area and
each IOMMU instance supports a mix of mappings with fixed da or not.
By changing the OMAP3 "isp" IOMMU pdata's da_start to 0x1000, we're
explicitly saying that iovm areas with fixed 'da' or not will never
map first page to OMAP3 ISP driver, what is the behavior it expects.
Without this patch, thanks to patch 1/3 the IOMMU driver will not map
address 0x0 to OMAP3 ISP anyway. But then ISP will be passing wrong
information that it would be fine to map first page. :)
Kind regards,
David Cohen
>
> Regards,
>
> --
> Sakari Ailus
> sakari.ai...@maxwell.research.nokia.com
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Hi Hiroshi, Fernando, On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando wrote: > On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU wrote: >> From: "ext Guzman Lugo, Fernando" >> Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set >> IOVMF_DA_FIXED/IOVMF_DA_ANON flags >> Date: Tue, 8 Mar 2011 11:59:43 -0600 >> >>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote: >>>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according >>>> to input 'da' address when mapping memory: >>>> da == 0: IOVMF_DA_ANON >>>> da != 0: IOVMF_DA_FIXED >>>> >>>> It prevents IOMMU to map first page with fixed 'da'. To avoid such >>>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now >>>> come from the user. IOVMF_DA_ANON will be automatically set if >>>> IOVMF_DA_FIXED isn't set. >>>> >>>> Signed-off-by: David Cohen >>>> --- >>>> arch/arm/plat-omap/iovmm.c | 12 >>>> 1 files changed, 8 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c >>>> index 11c9b76..dde9cb0 100644 >>>> --- a/arch/arm/plat-omap/iovmm.c >>>> +++ b/arch/arm/plat-omap/iovmm.c >>>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct >>>> sg_table *sgt, >>>> flags &= IOVMF_HW_MASK; >>>> flags |= IOVMF_DISCONT; >>>> flags |= IOVMF_MMIO; >>>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>>> + if (~flags & IOVMF_DA_FIXED) >>>> + flags |= IOVMF_DA_ANON; >>> >>> could we use only one? both are mutual exclusive, what happen if flag >>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >>> IOVMF_DA_ANON. >> >> Then, what about introducing some MACRO? Better names? >> >> #define set_iovmf_da_anon(flags) >> #define set_iovmf_da_fix(flags) >> #define set_iovmf_mmio(flags) > > will they be used by the users? > > I think people are more used to use > > iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); I'd be happier with this approach, instead of the macros. :) It's intuitive and very common on kernel. > > than > > set_iovmf_da_anon(flags) > set_iovmf_mmio(flags) > iommu_vmap(obj, da, sgt, flags); > > I don't have problem with the change, but I think how it is now is ok, > just that we don't we two bits to handle anon/fixed da, it can be > managed it only 1 bit (one flag), or is there a issue? We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. I can resend my patch if we agree it's OK. Regards, David > > Regards, > Fernando. >> .. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] omap: iovmm: disallow mapping NULL address
On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
wrote:
> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote:
>> From: Michael Jones
>>
>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> the NULL address if da_start==0, which would then not get unmapped.
>> Disallow this again. And spell variable 'alignment' correctly.
>>
>> Signed-off-by: Michael Jones
>> ---
>> arch/arm/plat-omap/iovmm.c | 16 ++--
>> 1 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>> index 6dc1296..11c9b76 100644
>> --- a/arch/arm/plat-omap/iovmm.c
>> +++ b/arch/arm/plat-omap/iovmm.c
>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct
>> iommu *obj, u32 da,
>> size_t bytes, u32 flags)
>> {
>> struct iovm_struct *new, *tmp;
>> - u32 start, prev_end, alignement;
>> + u32 start, prev_end, alignment;
>>
>> if (!obj || !bytes)
>> return ERR_PTR(-EINVAL);
>>
>> start = da;
>> - alignement = PAGE_SIZE;
>> + alignment = PAGE_SIZE;
>>
>> if (flags & IOVMF_DA_ANON) {
>> - start = obj->da_start;
>> + /* Don't map address 0 */
>> + if (obj->da_start)
>> + start = obj->da_start;
>> + else
>> + start = obj->da_start + alignment;
>
> else part obj->da_star is always 0, so why not?
> start = alignment;
The patch is not from me, but I can fix it if Michael agrees.
>
> so, why I understand, it now it is possible mapp address 0x0 only if
> IOVMF_DA_ANON is not set, right? maybe that would be mention in the
> patch.
Indeed address 0x0 was never meant to be mapped. The mentioned commit
on the patch's description did that without noticing. But the new
change is documented in the code by the comment "Don't map address 0"
and it's also mentioned on the patch description. Is it OK for you?
Regards,
David Cohen
>
> Regards,
> Fernando.
>
>>
>> if (flags & IOVMF_LINEAR)
>> - alignement = iopgsz_max(bytes);
>> - start = roundup(start, alignement);
>> + alignment = iopgsz_max(bytes);
>> + start = roundup(start, alignment);
>> } else if (start < obj->da_start || start > obj->da_end ||
>> obj->da_end - start < bytes) {
>> return ERR_PTR(-EINVAL);
>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
>> *obj, u32 da,
>> goto found;
>>
>> if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>> - start = roundup(tmp->da_end + 1, alignement);
>> + start = roundup(tmp->da_end + 1, alignment);
>>
>> prev_end = tmp->da_end;
>> }
>> --
>> 1.7.0.4
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
[snip] >>>>>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>>>>> + if (~flags & IOVMF_DA_FIXED) >>>>>> + flags |= IOVMF_DA_ANON; >>>>> >>>>> could we use only one? both are mutual exclusive, what happen if flag >>>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >>>>> IOVMF_DA_ANON. >>>> >>>> Then, what about introducing some MACRO? Better names? >>>> >>>> #define set_iovmf_da_anon(flags) >>>> #define set_iovmf_da_fix(flags) >>>> #define set_iovmf_mmio(flags) >>> >>> will they be used by the users? >>> >>> I think people are more used to use >>> >>> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); >> >> I'd be happier with this approach, instead of the macros. :) >> It's intuitive and very common on kernel. >> >>> >>> than >>> >>> set_iovmf_da_anon(flags) >>> set_iovmf_mmio(flags) >>> iommu_vmap(obj, da, sgt, flags); >>> >>> I don't have problem with the change, but I think how it is now is ok, >>> just that we don't we two bits to handle anon/fixed da, it can be >>> managed it only 1 bit (one flag), or is there a issue? >> >> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. >> I can resend my patch if we agree it's OK. > > sounds perfect to me. Not sure indeed if this change fits to this same patch. Looks like a 4th patch sounds better. Br, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
On Tue, Mar 8, 2011 at 9:46 PM, David Cohen wrote: > [snip] > >>>>>>> - flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON); >>>>>>> + if (~flags & IOVMF_DA_FIXED) >>>>>>> + flags |= IOVMF_DA_ANON; >>>>>> >>>>>> could we use only one? both are mutual exclusive, what happen if flag >>>>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of >>>>>> IOVMF_DA_ANON. >>>>> >>>>> Then, what about introducing some MACRO? Better names? >>>>> >>>>> #define set_iovmf_da_anon(flags) >>>>> #define set_iovmf_da_fix(flags) >>>>> #define set_iovmf_mmio(flags) >>>> >>>> will they be used by the users? >>>> >>>> I think people are more used to use >>>> >>>> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON); >>> >>> I'd be happier with this approach, instead of the macros. :) >>> It's intuitive and very common on kernel. >>> >>>> >>>> than >>>> >>>> set_iovmf_da_anon(flags) >>>> set_iovmf_mmio(flags) >>>> iommu_vmap(obj, da, sgt, flags); >>>> >>>> I don't have problem with the change, but I think how it is now is ok, >>>> just that we don't we two bits to handle anon/fixed da, it can be >>>> managed it only 1 bit (one flag), or is there a issue? >>> >>> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only. >>> I can resend my patch if we agree it's OK. >> >> sounds perfect to me. > > Not sure indeed if this change fits to this same patch. Looks like a > 4th patch sounds better. Indeed not. :) A new set is coming soon. Br, David > > Br, > > David Cohen > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] omap: iovmm: disallow mapping NULL address
On Tue, Mar 8, 2011 at 9:53 PM, Guzman Lugo, Fernando
wrote:
> On Tue, Mar 8, 2011 at 1:06 PM, David Cohen wrote:
>> On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
>> wrote:
>>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen wrote:
>>>> From: Michael Jones
>>>>
>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>> the NULL address if da_start==0, which would then not get unmapped.
>>>> Disallow this again. And spell variable 'alignment' correctly.
>>>>
>>>> Signed-off-by: Michael Jones
>>>> ---
>>>> arch/arm/plat-omap/iovmm.c | 16 ++--
>>>> 1 files changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>>> index 6dc1296..11c9b76 100644
>>>> --- a/arch/arm/plat-omap/iovmm.c
>>>> +++ b/arch/arm/plat-omap/iovmm.c
>>>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct
>>>> iommu *obj, u32 da,
>>>> size_t bytes, u32 flags)
>>>> {
>>>> struct iovm_struct *new, *tmp;
>>>> - u32 start, prev_end, alignement;
>>>> + u32 start, prev_end, alignment;
>>>>
>>>> if (!obj || !bytes)
>>>> return ERR_PTR(-EINVAL);
>>>>
>>>> start = da;
>>>> - alignement = PAGE_SIZE;
>>>> + alignment = PAGE_SIZE;
>>>>
>>>> if (flags & IOVMF_DA_ANON) {
>>>> - start = obj->da_start;
>>>> + /* Don't map address 0 */
>>>> + if (obj->da_start)
>>>> + start = obj->da_start;
>>>> + else
>>>> + start = obj->da_start + alignment;
>>>
>>> else part obj->da_star is always 0, so why not?
>>> start = alignment;
>>
>> The patch is not from me, but I can fix it if Michael agrees.
>>
>>>
>>> so, why I understand, it now it is possible mapp address 0x0 only if
>>> IOVMF_DA_ANON is not set, right? maybe that would be mention in the
>>> patch.
>>
>> Indeed address 0x0 was never meant to be mapped. The mentioned commit
>> on the patch's description did that without noticing. But the new
>> change is documented in the code by the comment "Don't map address 0"
>
> yeah, but it only applies when "flags & IOVMF_DA_ANON", So if I use
> IOVMF_DA_FIXED and da_start == 0, I will be able to map some area
> which starts from address 0x0, right? if so, that looks good to me
> and the description should mention that if is disallowing mapping
> address 0x0 when IOVMF_DA_ANON is used.
Yes, that's the case. I will update the comment. I hope Michael does
not complain about the changes. :)
Br,
David
>
> Regards,
> Fernando.
>
>> and it's also mentioned on the patch description. Is it OK for you?
>>
>> Regards,
>>
>> David Cohen
>>
>>>
>>> Regards,
>>> Fernando.
>>>
>>>>
>>>> if (flags & IOVMF_LINEAR)
>>>> - alignement = iopgsz_max(bytes);
>>>> - start = roundup(start, alignement);
>>>> + alignment = iopgsz_max(bytes);
>>>> + start = roundup(start, alignment);
>>>> } else if (start < obj->da_start || start > obj->da_end ||
>>>> obj->da_end - start < bytes) {
>>>> return ERR_PTR(-EINVAL);
>>>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct
>>>> iommu *obj, u32 da,
>>>> goto found;
>>>>
>>>> if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>>>> - start = roundup(tmp->da_end + 1, alignement);
>>>> + start = roundup(tmp->da_end + 1, alignment);
>>>>
>>>> prev_end = tmp->da_end;
>>>> }
>>>> --
>>>> 1.7.0.4
>>>>
>>>>
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] *** SUBJECT HERE ***
On Tue, Mar 8, 2011 at 10:04 PM, David Cohen wrote: > *** BLURB HERE *** Sorry for this garbage :/ Br, David > > David Cohen (2): > omap3: change ISP's IOMMU da_start address > omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag > > Michael Jones (1): > omap: iovmm: disallow mapping NULL address > > arch/arm/mach-omap2/omap-iommu.c | 2 +- > arch/arm/plat-omap/include/plat/iovmm.h | 2 -- > arch/arm/plat-omap/iovmm.c | 30 +++--- > 3 files changed, 16 insertions(+), 18 deletions(-) > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
IOVMM driver checks input 'da == 0' when mapping address to determine whether user wants fixed 'da' or not. At the same time, it doesn't disallow address 0x0 to be used, what creates an ambiguous situation. This patch set moves fixed 'da' check to the input flags. It also fixes da_start value for ISP IOMMU instance. Br, David Cohen --- David Cohen (2): omap3: change ISP's IOMMU da_start address omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag Michael Jones (1): omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set arch/arm/mach-omap2/omap-iommu.c|2 +- arch/arm/plat-omap/include/plat/iovmm.h |2 -- arch/arm/plat-omap/iovmm.c | 30 +++--- 3 files changed, 16 insertions(+), 18 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set
From: Michael Jones
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
address if da_start==0, which would then not get unmapped. Disallow
this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
correctly.
Signed-off-by: Michael Jones
---
arch/arm/plat-omap/iovmm.c | 16 ++--
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..e5f8341 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags & IOVMF_DA_ANON) {
- start = obj->da_start;
+ /* Don't map address 0 */
+ if (obj->da_start)
+ start = obj->da_start;
+ else
+ start = alignment;
if (flags & IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start < obj->da_start || start > obj->da_end ||
obj->da_end - start < bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
- start = roundup(tmp->da_end + 1, alignement);
+ start = roundup(tmp->da_end + 1, alignment);
prev_end = tmp->da_end;
}
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] omap3: change ISP's IOMMU da_start address
ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.
Signed-off-by: David Cohen
---
arch/arm/mach-omap2/omap-iommu.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
.name = "isp",
.nr_tlb_entries = 8,
.clk_name = "cam_ick",
- .da_start = 0x0,
+ .da_start = 0x1000,
.da_end = 0xF000,
},
},
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag
Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED
It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user throught 'flags' parameter when mapping memory.
As IOVMF_DA_ANON and IOVMF_DA_FIXED are mutually exclusive, IOVMF_DA_ANON
can be removed. The driver will now check internally if IOVMF_DA_FIXED
is set or not.
Signed-off-by: David Cohen
---
arch/arm/plat-omap/include/plat/iovmm.h |2 --
arch/arm/plat-omap/iovmm.c | 14 +-
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iovmm.h
b/arch/arm/plat-omap/include/plat/iovmm.h
index bdc7ce5..32a2f6c 100644
--- a/arch/arm/plat-omap/include/plat/iovmm.h
+++ b/arch/arm/plat-omap/include/plat/iovmm.h
@@ -71,8 +71,6 @@ struct iovm_struct {
#define IOVMF_LINEAR_MASK (3 << (2 + IOVMF_SW_SHIFT))
#define IOVMF_DA_FIXED (1 << (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_ANON (2 << (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_MASK (3 << (4 + IOVMF_SW_SHIFT))
extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index e5f8341..894489c 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -279,7 +279,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
start = da;
alignment = PAGE_SIZE;
- if (flags & IOVMF_DA_ANON) {
+ if (~flags & IOVMF_DA_FIXED) {
/* Don't map address 0 */
if (obj->da_start)
start = obj->da_start;
@@ -307,7 +307,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
if (tmp->da_start > start && (tmp->da_start - start) >= bytes)
goto found;
- if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
+ if (tmp->da_end >= start && ~flags & IOVMF_DA_FIXED)
start = roundup(tmp->da_end + 1, alignment);
prev_end = tmp->da_end;
@@ -654,7 +654,6 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
sg_table *sgt,
flags &= IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -694,7 +693,7 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-n-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -713,7 +712,6 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags &= IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
sgt = sgtable_alloc(bytes, flags, da, 0);
if (IS_ERR(sgt)) {
@@ -784,7 +782,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa,
void *va,
* @flags: iovma and page property
*
* Creates 1-1-1 mapping and returns @da again, which can be
- * adjusted if 'IOVMF_DA_ANON' is set.
+ * adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
u32 flags)
@@ -803,7 +801,6 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
bytes,
flags &= IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -842,7 +839,7 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-1-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -862,7 +859,6 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags &= IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap: iommu: disallow mapping NULL address
On Tue, Mar 8, 2011 at 10:31 PM, Laurent Pinchart wrote: > Hi David, > > On Monday 07 March 2011 22:35:31 David Cohen wrote: >> On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote: >> > On Monday 07 March 2011 20:41:21 David Cohen wrote: >> >> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote: >> >> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote: >> >> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote: >> >> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote: >> >> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 >> >> >>>> 2001 From: Michael Jones >> >> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100 >> >> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address >> >> >>>> >> >> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping >> >> >>>> the NULL address if da_start==0. Force da_start to exclude the >> >> >>>> first page. >> >> >>> >> >> >>> what about devices that uses page 0? ipu after reset always starts >> >> >>> from 0x how could we map that address?? >> >> >> >> >> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you >> >> >> want it? >> >> > >> >> > unlike DSP that you can load a register with the addres the DSP will >> >> > boot, IPU core always starts from address 0x, so if you take >> >> > IPU out of reset it will try to access address 0x0 if not map it, >> >> > there will be a mmu fault. >> >> >> >> Hm. Looks like the iommu should not restrict any da. The valid da >> >> range should rely only on pdata. >> >> Michael, what about just update ISP's da_start on omap-iommu.c file? >> >> Set it to 0x1000. >> > >> > What about patching the OMAP3 ISP driver to use a non-zero value (maybe >> > -1) as an invalid/freed pointer ? >> >> I wouldn't be comfortable to use 0 (or NULL) value as valid address on >> ISP driver. > > Why not ? The IOMMUs can use 0x as a valid address. Whether we allow > it or not is a software architecture decision, not influenced by the IOMMU > hardware. As some peripherals (namely IPU) require mapping memory to > 0x, the IOMMU layer must support it and not treat 0x > specially. All da == 0 checks to aim at catching invalid address values must > be removed, both from the IOMMU API and the IOMMU internals. Yes, it can use and IOMMU should not treat is specially. That's the aim of my patch: [PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag I'm not advocating to not allow 0x0, but to not use it when user is not requesting fixed da. In many sw architecture decisions 0x0 address is a special case. To avoid any misuse, IOMMU will not use it unless it's requested. If user is not requesting fixed 'da', it's not a problem to not give 0x0 anyway. IMO that's the safer option for all cases. > >> The 'da' range (da_start and da_end) is defined per VM and specified as >> platform data. IMO, to set da_start = 0x1000 seems to be> a correct approach >> for ISP as it's the only client for its IOMMU instance. > > We can do that, and then use 0 as an invalid pointer in the ISP driver. As the > IOMMU API will use another value (what about 0x, as for the userspace > mmap() call ?) to mean "invalid pointer", it might be better to use the same > value in the ISP driver. That can be done, of course. But the main point is in OMAP3 ISP all initial register values to read/write from/to memory are 0x0. It means sometimes we can catch bugs more easily by not mapping that address. So, IMO, OMAP3 ISP should not allow to map first page. But that's a special case for this driver only. Br, David > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] omap3: change ISP's IOMMU da_start address
On Wed, Mar 9, 2011 at 9:43 AM, Sakari Ailus
wrote:
> David Cohen wrote:
>> ISP doesn't consider 0x0 as a valid address, so it should explicitly
>> exclude first page from allowed 'da' range.
>>
>> Signed-off-by: David Cohen
>> ---
>> arch/arm/mach-omap2/omap-iommu.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap-iommu.c
>> b/arch/arm/mach-omap2/omap-iommu.c
>> index 3fc5dc7..3bea489 100644
>> --- a/arch/arm/mach-omap2/omap-iommu.c
>> +++ b/arch/arm/mach-omap2/omap-iommu.c
>> @@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
>> .name = "isp",
>> .nr_tlb_entries = 8,
>> .clk_name = "cam_ick",
>> - .da_start = 0x0,
>> + .da_start = 0x1000,
>> .da_end = 0xF000,
>> },
>> },
>
> Hi David!
Hi Sakari,
>
> Thanks for the patch.
And thanks for the comments. :)
>
> My question is once again: is this necessary? My understanding is that
> the IOMMU allows mapping the NULL address if the user wishes to map it
> explicitly. da_end specifies the real hardware limit for the mapped top
> address, da_start should do the same for bottom.
Hm. da_start/da_end in this case belong to OMAP3 IOMMU ISP VM. It
means they're related to OMAP3 ISP only. But they do not reflect the
hw limit, as the range limit is anything which fits in u32. They say
what's the range OMAP3 ISP is expecting to have mapped. And the answer
to this question is the first page is not expected. Then, why say the
opposite in da_start?
>
> I think that the IOMMU users should be either able to rely that they get
> no NULL allocated automatically for them. Do we want or not want it to
> be part of the API? I don't think the ISP driver is a special case of
> all the possible drivers using the IOMMU.
My understanding after this discussion is address 0x0 should be
allowed (what is done by patch 3/3 of this set). But as a safer
choice, it won't be returned without explicitly request (what is done
in path 1/3). Because of that, I'm OK in drop this patch 2/3. But then
there's one question which is the motivation for this change:
If the current OMAP3 ISP driver doesn't want the first page, (1)
should we pass a generic information and expect IOVMM driver to
correct it to ISP need or (2) should we pass the information which
reflects the real ISP driver's need?
My understanding is (2). But I'm fine with (1) as it will lead to the
same result.
>
> On the other hand, probably there will be an API change at some point
> for the IOMMU since as far as I remember, there are somewhat
> established APIs for IOMMUs in existence.
At some point we need to find a standard for many IOMMU drivers. But
for now we need to stick with what we have. :)
Regards,
David Cohen
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/2] omap: iovmm: Fix IOVMM check for fixed 'da'
Hi, Previous patch 2/3 was dropped in this new version. Patch 1 was updated according to a comment it got. --- IOVMM driver checks input 'da == 0' when mapping address to determine whether user wants fixed 'da' or not. At the same time, it doesn't disallow address 0x0 to be used, what creates an ambiguous situation. This patch set moves fixed 'da' check to the input flags. Br, David Cohen --- David Cohen (1): omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag Michael Jones (1): omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set arch/arm/plat-omap/include/plat/iovmm.h |2 -- arch/arm/plat-omap/iovmm.c | 27 --- 2 files changed, 12 insertions(+), 17 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set
From: Michael Jones
commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
address if da_start==0, which would then not get unmapped. Disallow
this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
correctly.
Signed-off-by: Michael Jones
---
arch/arm/plat-omap/iovmm.c | 13 +++--
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..ea7eab9 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,21 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
size_t bytes, u32 flags)
{
struct iovm_struct *new, *tmp;
- u32 start, prev_end, alignement;
+ u32 start, prev_end, alignment;
if (!obj || !bytes)
return ERR_PTR(-EINVAL);
start = da;
- alignement = PAGE_SIZE;
+ alignment = PAGE_SIZE;
if (flags & IOVMF_DA_ANON) {
- start = obj->da_start;
+ /* Don't map address 0 */
+ start = obj->da_start ? obj->da_start : alignment;
if (flags & IOVMF_LINEAR)
- alignement = iopgsz_max(bytes);
- start = roundup(start, alignement);
+ alignment = iopgsz_max(bytes);
+ start = roundup(start, alignment);
} else if (start < obj->da_start || start > obj->da_end ||
obj->da_end - start < bytes) {
return ERR_PTR(-EINVAL);
@@ -304,7 +305,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
goto found;
if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
- start = roundup(tmp->da_end + 1, alignement);
+ start = roundup(tmp->da_end + 1, alignment);
prev_end = tmp->da_end;
}
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag
Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED
It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user throught 'flags' parameter when mapping memory.
As IOVMF_DA_ANON and IOVMF_DA_FIXED are mutually exclusive, IOVMF_DA_ANON
can be removed. The driver will now check internally if IOVMF_DA_FIXED
is set or not.
Signed-off-by: David Cohen
---
arch/arm/plat-omap/include/plat/iovmm.h |2 --
arch/arm/plat-omap/iovmm.c | 14 +-
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iovmm.h
b/arch/arm/plat-omap/include/plat/iovmm.h
index bdc7ce5..32a2f6c 100644
--- a/arch/arm/plat-omap/include/plat/iovmm.h
+++ b/arch/arm/plat-omap/include/plat/iovmm.h
@@ -71,8 +71,6 @@ struct iovm_struct {
#define IOVMF_LINEAR_MASK (3 << (2 + IOVMF_SW_SHIFT))
#define IOVMF_DA_FIXED (1 << (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_ANON (2 << (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_MASK (3 << (4 + IOVMF_SW_SHIFT))
extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index ea7eab9..51ef43e 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -279,7 +279,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
start = da;
alignment = PAGE_SIZE;
- if (flags & IOVMF_DA_ANON) {
+ if (~flags & IOVMF_DA_FIXED) {
/* Don't map address 0 */
start = obj->da_start ? obj->da_start : alignment;
@@ -304,7 +304,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu
*obj, u32 da,
if (tmp->da_start > start && (tmp->da_start - start) >= bytes)
goto found;
- if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
+ if (tmp->da_end >= start && ~flags & IOVMF_DA_FIXED)
start = roundup(tmp->da_end + 1, alignment);
prev_end = tmp->da_end;
@@ -651,7 +651,6 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct
sg_table *sgt,
flags &= IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -691,7 +690,7 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-n-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -710,7 +709,6 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags &= IOVMF_HW_MASK;
flags |= IOVMF_DISCONT;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
sgt = sgtable_alloc(bytes, flags, da, 0);
if (IS_ERR(sgt)) {
@@ -781,7 +779,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa,
void *va,
* @flags: iovma and page property
*
* Creates 1-1-1 mapping and returns @da again, which can be
- * adjusted if 'IOVMF_DA_ANON' is set.
+ * adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
u32 flags)
@@ -800,7 +798,6 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t
bytes,
flags &= IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
@@ -839,7 +836,7 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
* @flags: iovma and page property
*
* Allocate @bytes linearly and creates 1-1-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
{
@@ -859,7 +856,6 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
u32 flags)
flags &= IOVMF_HW_MASK;
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
da = __iommu_kmap(obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: OMAP 3430 Camera/ISP out of memory error
Hi, On Fri, Mar 25, 2011 at 1:34 PM, Patrick Radius wrote: > Ok, thanks! > However, I'm also quite curious about peoples thoughts about it from > here. > Since I think what's happing is quite OMAP specific. > For example, I was wondering where omap34xxcam.c gets it's memory it > should allocate from. > Is it the 'main' memory? Or does it share memory with a framebuffer > (overlay)? Or memory from the (M-4MO) sensor? > Is it possible I should allocate memory for it as boot argument? similar > like vmem=16M omapfb.vram=0:8M > > I can't find much information about this all... You're using an old version of the driver. Please, use the newer one available in mainline. Regards, David Cohen > > On vr, 2011-03-25 at 13:24 +0200, Sakari Ailus wrote: >> Patrick Radius wrote: >> > Hi, >> >> Hi Patrick, >> >> I think this question will get better answered in the linux-media list. >> Cc it. >> >> > i'm trying to get camera support working on a relatively new Android >> > port to an OMAP 3430 based phone (Samsung GT-i8320 a.k.a. Samsung H1). >> > However calls to VIDIOC_REQBUFS fail with a -12 (OUT OF MEMORY). >> > As far as I can see this return error isn't even supposed to exist, >> > according to the V4L2 spec. >> > I'm using the code from Android on the Zoom2. >> > The sensor is a Fujitsu M-4MO. >> > >> > Any ideas on what could be wrong with the out of memory result? >> >> First of all, which version of the driver are you using, i.e. is it the >> current one going to upstream? >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
On Fri, Mar 25, 2011 at 5:17 PM, Sakari Ailus wrote: > Hi, Hi Sakari, > > [Resend: the patches were accidentally sent to linux-media instead. > Apologies.] > > This patchset is aimed to fix a problem in arch_iommu implementation > references. When an actual arch_iommu implementation is not loaded while > iommu_get() is being called results to a kernel oops, as well as > removing an arch_iommu implementation which is in use. > > This patchset fixes both issues. Sounds nice. > > The patchset assumes the arch_iommu is uninstalled at module unload > time. Is this an acceptable requirement? I can't see a reason why this assumption could be wrong. In my point of view it's acceptable. Let's see Hiroshi's. Regards, David Cohen > > Serialisation of the access to arch_iommu is done using mutex called > arch_iommu_mutex. > > module_put() doesn't need to have the arch_iommu_mutex since when this > gets called there won't be any users on the arch_iommu anyway. > > Comments are welcome. :-) > > Cheers, > > -- > Sakari Ailus > sakari.ai...@maxwell.research.nokia.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: OMAP 3430 Camera/ISP out of memory error
On Fri, Mar 25, 2011 at 9:01 PM, Patrick Radius wrote: > Thanks, > > I'm looking at > http://gitorious.org/linux-omap/mainline/trees/master/drivers/media/video > > however, I can't seem to find the drivers I need. > I only see omap24xxcam while I was using the omap34xxcam from Linux > kernel 2.6.32.9. > And I also don't see any M-4MO driver anymore. > I'm probably looking at the wrong spot, but... You need to get an up-to-date mainline kernel. The driver is in the directory: drivers/media/video/omap3isp/ I don't recommend you to use 2.6.32, as it depends on some new V4L2 stuff which you won't natively find in such old kernel version. I also suggest you to CC linux-me...@vger.kernel.org as well in next questions. Br, David > > As you can tell, I'm fairly new with OMAP programming. > > Thanks! > > Regards, > Patrick > > On vr, 2011-03-25 at 16:34 +0200, David Cohen wrote: >> Hi, >> >> On Fri, Mar 25, 2011 at 1:34 PM, Patrick Radius wrote: >> > Ok, thanks! >> > However, I'm also quite curious about peoples thoughts about it from >> > here. >> > Since I think what's happing is quite OMAP specific. >> > For example, I was wondering where omap34xxcam.c gets it's memory it >> > should allocate from. >> > Is it the 'main' memory? Or does it share memory with a framebuffer >> > (overlay)? Or memory from the (M-4MO) sensor? >> > Is it possible I should allocate memory for it as boot argument? similar >> > like vmem=16M omapfb.vram=0:8M >> > >> > I can't find much information about this all... >> >> You're using an old version of the driver. Please, use the newer one >> available in mainline. >> >> Regards, >> >> David Cohen >> >> > >> > On vr, 2011-03-25 at 13:24 +0200, Sakari Ailus wrote: >> >> Patrick Radius wrote: >> >> > Hi, >> >> >> >> Hi Patrick, >> >> >> >> I think this question will get better answered in the linux-media list. >> >> Cc it. >> >> >> >> > i'm trying to get camera support working on a relatively new Android >> >> > port to an OMAP 3430 based phone (Samsung GT-i8320 a.k.a. Samsung H1). >> >> > However calls to VIDIOC_REQBUFS fail with a -12 (OUT OF MEMORY). >> >> > As far as I can see this return error isn't even supposed to exist, >> >> > according to the V4L2 spec. >> >> > I'm using the code from Android on the Zoom2. >> >> > The sensor is a Fujitsu M-4MO. >> >> > >> >> > Any ideas on what could be wrong with the out of memory result? >> >> >> >> First of all, which version of the driver are you using, i.e. is it the >> >> current one going to upstream? >> >> >> > >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> > the body of a message to majord...@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
Hi Sakari, On Sun, Mar 27, 2011 at 8:27 PM, Sakari Ailus wrote: > Ramirez Luna, Omar wrote: >> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus >> wrote: >>> Hi, >>> >>> This patchset is aimed to fix a problem in arch_iommu implementation >>> references. When an actual arch_iommu implementation is not loaded while >>> iommu_get() is being called results to a kernel oops, as well as >>> removing an arch_iommu implementation which is in use. >> >> How about fixing the dependency instead? Right now iommu2 depends on >> iommu because of the calls to >> install_iommu_arch/uninstall_iommu_arch... we should change that >> dependency to iommu depend on iommu2. Something like iommu (plat) >> querying iommu2 (mach) for devices to install. > > There is no direct dependency from a driver using the generic API to a > particular implementation of the iommu. This comes from the design of > the iommu framework. The generic layer shouldn't depend on particular > implementation(s). > > What comes to the patch, it works as long as there's only one iommu > implementation loaded / compiled to the kernel. I wonder if this kind of > limitation can be accepted. The generic iommu driver cannot support more than one implementation loaded at the same time, so your patch is correct by assuming it. Regards, David Cohen > > Regards, > > -- > Sakari Ailus > sakari.ai...@maxwell.research.nokia.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
On Mon, Mar 28, 2011 at 4:42 AM, Ramirez Luna, Omar wrote: > Hi, > > On Sun, Mar 27, 2011 at 12:27 PM, Sakari Ailus > wrote: >> Ramirez Luna, Omar wrote: >>> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus >>> wrote: >>>> This patchset is aimed to fix a problem in arch_iommu implementation >>>> references. When an actual arch_iommu implementation is not loaded while >>>> iommu_get() is being called results to a kernel oops, as well as >>>> removing an arch_iommu implementation which is in use. >>> >>> How about fixing the dependency instead? Right now iommu2 depends on >>> iommu because of the calls to >>> install_iommu_arch/uninstall_iommu_arch... we should change that >>> dependency to iommu depend on iommu2. Something like iommu (plat) >>> querying iommu2 (mach) for devices to install. >> >> There is no direct dependency from a driver using the generic API to a >> particular implementation of the iommu. This comes from the design of >> the iommu framework. The generic layer shouldn't depend on particular >> implementation(s). > > IMHO there is, take as an example bridgedriver (it is arm/omap > dependent), so it depends on iommu providing the mach-omap2 > implementation. I imagine isp for omap imposes the same dependency, > even more your patchset enforces that dependency. The generic layer can exist without a specific implementation. It should accept later arch install/uninstall without any problem. > > Basically, if there is no "arch_iommu" the iommu driver does not work, > and if there was an "arch_iommu" but it was removed then the driver > crashes. The loaded module *must* uninstall arch when existing, then kernel will see no crash. And yes, generic layer won't work anymore but that's an expected and correct situation. > > Now, there could be architectures that does not depend on a particular > implementation but this iommu driver doesn't support them because if > there is no arch_iommu operations, it does nothing or crashes. > >> What comes to the patch, it works as long as there's only one iommu >> implementation loaded / compiled to the kernel. I wonder if this kind of >> limitation can be accepted. > > Which is the way iommu choose to work, like I said if there is no > arch_iommu nothing works, most of APIs in iommu depend on an machine > specific implementation. To fix that it is not the scope of my > proposal. > > If indeed iommu can function without a machine specific implementation > then a redesign needs to be made, but to me the same approach as I did > needs to be followed: if there is mach implementation (e.g.: iommu2.c) > the generic API needs to depend on it, otherwise the module can be > removed and crash the kernel; OTOH if there is no mach implementation, > then iommu should not depend on it to be installed as you point out, > this could be handled in plat/iommu.h among with: > > #if defined(CONFIG_ARCH_OMAP1) > #error "iommu for this processor not implemented yet" > #else > #include > #endif So, every new specific implementation should modify this piece of code? Are you sure it's a good idea? Regards, David Cohen > > A new else defining the install/uninstall_arch_iommu functions or > simply reversing the check to be OMAP2+ and error on anything else. > > Regards, > > Omar > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
On Wed, Mar 30, 2011 at 4:56 PM, Laurent Pinchart wrote: > On Wednesday 30 March 2011 15:50:37 Sakari Ailus wrote: >> Laurent Pinchart wrote: >> > Hi Sakari, >> >> Hi Laurent, >> >> > On Wednesday 30 March 2011 10:16:56 Sakari Ailus wrote: >> >> Laurent Pinchart wrote: >> >>> On Friday 25 March 2011 20:37:55 Ramirez Luna, Omar wrote: >> On Fri, Mar 25, 2011 at 10:13 AM, Sakari Ailus wrote: >> > Hi, >> > >> > This patchset is aimed to fix a problem in arch_iommu implementation >> > references. When an actual arch_iommu implementation is not loaded >> > while iommu_get() is being called results to a kernel oops, as well >> > as removing an arch_iommu implementation which is in use. >> >> How about fixing the dependency instead? Right now iommu2 depends on >> iommu because of the calls to >> install_iommu_arch/uninstall_iommu_arch... we should change that >> dependency to iommu depend on iommu2. Something like iommu (plat) >> querying iommu2 (mach) for devices to install. >> >>> >> >>> The reason why iommu depends on iommu2 and not the other way around is >> >>> because several mach-specific iommu implementations should be able to >> >>> coexist in the same kernel. The right one should be loaded at runtime. >> >>> >> >>> I think that Sakari's patches correcty fix the problems he noticed. >> >>> However, they won't fix one basic issue, which is that the iommu2 >> >>> module won't be automatically pulled in when the omap3isp module is >> >>> loaded. The omap3isp driver will then fail to probe the device. That's >> >>> better than crashing though. >> >> >> >> One option would be to specify the name of the module in the platform >> >> data and request_module() that in omap_iommu_probe(). This would solve >> >> the issue, not sure how pretty is this though. >> > >> > Do we need that ? My understanding is that a machine will need a single >> > mach- specific iommu implementation only. Drivers shouldn't need to care >> > about that. >> >> Well, no more than that there would have to be a driver for the IOMMU >> for that very hardware. >> >> > The iommu implementation should be automatically selected based on the >> > machine time. >> >> Machine type? >> >> I agree, but where is the selection made? > > The selection can be made by board code, or by the iommu implementations > themselves if they're compiled in. I prefer the first option. The second one will make the current implementation be even more OMAP-only. We have basically 3 layers: IOVMM, IOMMU_GENERIC and IOMMU_SPECIFIC. The middle one should be generic and don't care about machine types. The later one can be handled by board code as it's machine specific and, for most of the cases, I see no reason to let any other implementation besides the machine type's to be loaded. But the generic layer should not depend on any specific one. If somebody decides to load the specific layer after the generic one, it cannot be a problem. Regards, David > > -- > Regards, > > Laurent Pinchart > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
ght now it does >> push this to direction I don't like too much. The next implementer has >> to solve the problem instead, and it might be easier to implement this >> right now, as we are all up-to-date with the issue. > > We only have iommu2.ko at the moment. I've heard about an iommu1.ko being > worked on, but I don't have more information. We don't know whether the OMAP5 > will be able to use the same IOMMU implementation. Without more information, > I'm quite reluctant to design and implement a generic solution that will end > up being useless because we missed information in the design stage. One implementation belongs to mach-omap1 and other to mach-omap2. Not sure if it's a good plan to get them together. IMO the third option from Laurent solves the issue for now and don't make it more difficult to implement a better standard to OMAP. Regards, David Cohen -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] IOMMU error message cleanup
From: David Cohen Hi, OMAP IOMMU prints error messages twice. These patches remove the error message from the OMAP2,3 specific implementation and let them to be printed on the above layer only. Br, David --- David Cohen (2): OMAP: Add generic IOMMU errors code OMAP: Cleanup IOMMU error messages arch/arm/mach-omap2/iommu2.c| 33 +-- arch/arm/plat-omap/include/plat/iommu.h |9 +++- arch/arm/plat-omap/iommu.c | 36 -- 3 files changed, 48 insertions(+), 30 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] OMAP: Cleanup IOMMU error messages
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.
Signed-off-by: David Cohen
---
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",
- };
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;
iommu_write_reg(obj, stat, MMU_IRQSTATUS);
diff --git a/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index c653fd7..267c5b5 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -83,7 +83,7 @@ struct iommu_functions {
int (*enable)(struct iommu *obj);
void (*disable)(struct iommu *obj);
void (*set_twl)(struct iommu *obj, bool on);
- u32 (*fault_isr)(struct iommu *obj, u32 *ra);
+ u32 (*fault_isr)(struct iommu *obj, u32 *ra, u32 *iommu_errs);
void (*tlb_read_cr)(struct iommu *obj, struct cr_regs *cr);
void (*tlb_load_cr)(struct iommu *obj, struct cr_regs *cr);
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index b1107c0..c7c37a0 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -163,9 +163,9 @@ static u32 get_iopte_attr(struct iotlb_entry *e)
return arch_iommu->get_pte_attr(e);
}
-static u32 iommu_report_fault(struct iommu *obj, u32 *da)
+static u32 iommu_report_fault(struct iommu *obj, u32 *da, u32 *iommu_errs)
{
- return arch_iommu->fault_isr(obj, da);
+ return arch_iommu->fault_isr(obj, da, iommu_errs);
}
static void iotlb_lock_get(struct iommu *obj, struct iotlb_lock *l)
@@ -780,10 +780,18 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
*/
static irqreturn_t iommu_fault_handler(int irq, void *data)
{
- u32 stat, da;
+ int i;
+ u32 stat, da, errs;
u32 *iopgd, *iopte;
int err = -EIO;
struct iommu *obj = data;
+ const char *err_msg[] = {
+ "tlb miss",
+ "translation fault",
+ "emulation miss",
+ "table walk fault",
+ "multi hit fault",
+ };
if (!obj->refcount)
return IRQ_NONE;
@@ -796,7 +804,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
return IRQ_HANDLED;
clk_enable(obj->clk);
- stat = iommu_report_fault(obj, &da);
+ stat = iommu_report_fault(obj, &da, &errs);
clk_disable(obj->clk);
if (!stat)
return IRQ_HANDLED;
@@ -805,16 +813,20 @@ static irqreturn_t iommu_fault_handler(int irq, void
*data)
iopgd = iopgd_offset(obj, da);
- if (!iopgd_is_table(*iopgd)) {
- dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n", __func__,
- da, iopgd, *iopgd);
- return IRQ_NONE;
+ if (iopgd_is_table(*iopgd)) {
+ iopte = iopte_offs
[PATCH 1/2] OMAP: Add generic IOMMU errors code
Generic IOMMU errors code are necessary to handle errors on generic
layer.
Signed-off-by: David Cohen
---
arch/arm/plat-omap/include/plat/iommu.h |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 69230d6..c653fd7 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -109,6 +109,13 @@ struct iommu_platform_data {
u32 da_end;
};
+/* IOMMU errors */
+#define IOMMU_ERR_TLB_MISS (1 << 0)
+#define IOMMU_ERR_TRANS_FAULT (1 << 1)
+#define IOMMU_ERR_EMU_MISS (1 << 2)
+#define IOMMU_ERR_TBLWALK_FAULT(1 << 3)
+#define IOMMU_ERR_MULTIHIT_FAULT (1 << 4)
+
#if defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet"
#else
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] OMAP: Cleanup IOMMU error messages
On Mon, Jan 31, 2011 at 09:16:30PM -0600, ext Ramirez Luna, Omar wrote:
> Hi David,
Hi Ramirez,
Thanks for the comments.
>
> On Mon, Jan 31, 2011 at 11:25 AM, David Cohen 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.
>
A little bit of my context:
I'm working on OMAP3ISP driver. Due to hw or sw issues, iommu errors
(mainly iommu faults) are not so rare to happen. e.g. I have 2 hw issues
being workarounded on ISP H3A submodule (one of those a bit serious)
which may lead to iommu faults.
With the current implementation, the error messages are a bit useless
and much "heavy". For each "fault", 2 error lines are printed to show the
same error. On ISP context, usually it will happen *many* times in a
row, making the device not usable even to debug the issue. And the bad
side is we cannot get anything out of those messages which could help to
point out where is the problem. As sometimes it's difficult to reproduce it,
we loose a good chance to show a bit more useful data about the triggered
error.
My main intentions are:
- Get rid of one line.
- TODO: Print something useful to let developers to *know* what is
going on.
- TODO: Avoid to flood console to let it able to debug.
> 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?
Yes.
>
> > Signed-off-by: David Cohen
> > ---
> > 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?
That's true. I can work on a different approach.
>
> > 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 val
[PATCH] omap iommu: print module name on error messages
OMAP IOMMU generic layer doesn't need ot print function name during
error messages. Print module name instead which is more useful.
Signed-off-by: David Cohen
---
arch/arm/plat-omap/iommu.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index b1107c0..f55f458 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -806,7 +806,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
iopgd = iopgd_offset(obj, da);
if (!iopgd_is_table(*iopgd)) {
- dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n", __func__,
+ dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x\n", obj->name,
da, iopgd, *iopgd);
return IRQ_NONE;
}
@@ -814,7 +814,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
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);
+ obj->name, da, iopgd, *iopgd, iopte, *iopte);
return IRQ_NONE;
}
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 0/3] IOMMU fault callback support
Hi, These are RFC patches. They're intended to add fault callback support so IOMMU users can debug or react when a fault happens. IOMMU faults might be very difficult to reproduce and then to figure out the source of the problem. Currently IOMMU driver prints not so useful debug message and does not notice user about such issue. With a fault callback, IOMMU user may debug much more useful information and/or react to go back to a valid state or, in worse cases, to leave device in a state where it's still possible to use it and proceed with some investigation. Comments are welcome. Br, David --- David Cohen (3): OMAP: IOMMU: Add generic IOMMU errors code OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg() OMAP: IOMMU: add support to callback during fault handling arch/arm/mach-omap2/iommu2.c| 23 ++--- arch/arm/plat-omap/include/plat/iommu.h | 15 ++- arch/arm/plat-omap/iommu.c | 41 --- 3 files changed, 70 insertions(+), 9 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 1/3] OMAP: IOMMU: Add generic IOMMU errors code
Generic IOMMU errors code are necessary to handle errors on generic
layer.
Signed-off-by: David Cohen
---
arch/arm/plat-omap/include/plat/iommu.h |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 69230d6..71f369d 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -109,6 +109,13 @@ struct iommu_platform_data {
u32 da_end;
};
+/* IOMMU errors */
+#define OMAP_IOMMU_ERR_TLB_MISS(1 << 0)
+#define OMAP_IOMMU_ERR_TRANS_FAULT (1 << 1)
+#define OMAP_IOMMU_ERR_EMU_MISS(1 << 2)
+#define OMAP_IOMMU_ERR_TBLWALK_FAULT (1 << 3)
+#define OMAP_IOMMU_ERR_MULTIHIT_FAULT (1 << 4)
+
#if defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet"
#else
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 2/3] OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.
Signed-off-by: David Cohen
---
arch/arm/mach-omap2/iommu2.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..641f54a 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -163,7 +163,7 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
da = iommu_read_reg(obj, MMU_FAULT_AD);
*ra = da;
- dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);
for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
if (stat & (1 << i))
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 3/3] OMAP: IOMMU: add support to callback during fault handling
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
---
arch/arm/mach-omap2/iommu2.c| 21 +--
arch/arm/plat-omap/include/plat/iommu.h |8 +-
arch/arm/plat-omap/iommu.c | 41 ---
3 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 641f54a..d3f7871 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 71f369d..9e8c104 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 intrefcount;
struct mutexiommu_lock; /* global for this whole object */
@@ -48,6 +49,7 @@ struct iommu {
struct mutexmmap_lock; /* protect mmap */
int (*isr)(struct iommu *obj);
+ void (*fault_cb)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
void *ctx; /* iommu context: registres saved area */
u32 da_start;
@@ -83,7 +85,7 @@ struct iommu_functions {
int (*enable)(struct iommu *obj);
void (*disable)(struct iommu *obj);
void (*set_twl)(struct iommu *obj, bool on);
- u32 (*fault_isr)(struct iommu *obj, u32 *ra);
+ u32 (*fault_isr)(struct iommu *obj, u32 *ra, u32 *iommu_errs);
void (*tlb_read_cr)(struct iommu *obj, struct cr_regs *cr);
void (*tlb_load_cr)(struct iommu *obj, struct cr_regs *cr);
@@ -166,6 +168,10 @@ extern size_t iopgtable_clear_entry(struct iommu *obj, u32
iova);
extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end);
extern struct iommu *iommu_get(const char *name);
extern void iommu_put(struct iommu *obj);
+extern int iommu_set_fault_callback(const char *name,
+ void (*fault_cb)(struct iommu *obj, u32 da,
+u32 errs, void *priv),
+ void *fault_cb_priv);
extern void iommu_save_ctx(struct iommu *obj);
extern void iommu_restore_ctx(struct iommu *obj);
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index f55f458..7761eab 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -163,9 +163,9 @@ static u32 get_iopte_attr(struct iotlb_entry *e)
return arch_iommu->get_pte_attr(e);
}
-static u32 iommu_report_fault(struct iommu *obj, u32 *da)
+static u32 iommu_report_fault(struct iommu *obj, u32 *da, u32 *iommu_errs)
{
- return arch_iommu->fault_isr(obj, da);
+ return arch_iommu->fault_isr(obj, da, iommu_errs);
}
static void iotlb_lock_get(struct iommu *obj, struct iotlb_lock *l)
@@ -780,7 +780,7 @@ static void iopgtable_clear_entry_all(struct iommu *obj)
*/
static irqreturn_t iommu_fault_handler(int irq, void *data)
{
- u32 stat, da;
+ u32 stat, da, errs;
u32 *iopgd, *iopte;
int err = -EIO;
struct iommu *obj = data;
@@ -796,11 +796,17 @@ static irqreturn_t iommu_fault_handler(int irq, void
*dat
[PATCH] OMAP: IOMMU: add missing function declaration
Function declaration 'iopgtable_lookup_entry' is missing from header file. Signed-off-by: David Cohen --- arch/arm/plat-omap/include/plat/iommu.h |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h index 9e8c104..5a2475f 100644 --- a/arch/arm/plat-omap/include/plat/iommu.h +++ b/arch/arm/plat-omap/include/plat/iommu.h @@ -163,6 +163,8 @@ extern void flush_iotlb_range(struct iommu *obj, u32 start, u32 end); extern void flush_iotlb_all(struct iommu *obj); extern int iopgtable_store_entry(struct iommu *obj, struct iotlb_entry *e); +extern void iopgtable_lookup_entry(struct iommu *obj, u32 da, u32 **ppgd, + u32 **ppte); extern size_t iopgtable_clear_entry(struct iommu *obj, u32 iova); extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end); -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP: IOMMU: add missing function declaration
On Mon, Feb 14, 2011 at 12:36 AM, Varadarajan, Charulatha wrote: > On Sat, Feb 12, 2011 at 19:09, David Cohen wrote: >> Function declaration 'iopgtable_lookup_entry' is missing from header >> file. > > Is this done because of any error? > You may add a comment on why this is required. The function is exported by iommu module, but no other driver is using it so far. So, it's wrong but currently we have only a sparse warning. I can add this information to the patch description. Br, David > >> >> Signed-off-by: David Cohen >> --- >> arch/arm/plat-omap/include/plat/iommu.h | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/plat/iommu.h >> b/arch/arm/plat-omap/include/plat/iommu.h >> index 9e8c104..5a2475f 100644 >> --- a/arch/arm/plat-omap/include/plat/iommu.h >> +++ b/arch/arm/plat-omap/include/plat/iommu.h >> @@ -163,6 +163,8 @@ extern void flush_iotlb_range(struct iommu *obj, u32 >> start, u32 end); >> extern void flush_iotlb_all(struct iommu *obj); >> >> extern int iopgtable_store_entry(struct iommu *obj, struct iotlb_entry *e); >> +extern void iopgtable_lookup_entry(struct iommu *obj, u32 da, u32 **ppgd, >> + u32 **ppte); >> extern size_t iopgtable_clear_entry(struct iommu *obj, u32 iova); >> >> extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end); >> -- >> 1.7.1 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/3] OMAP: IOMMU: Add generic IOMMU errors code
On Mon, Feb 14, 2011 at 12:33 AM, Varadarajan, Charulatha wrote:
> On Sat, Feb 12, 2011 at 18:42, David Cohen wrote:
>> Generic IOMMU errors code are necessary to handle errors on generic
>> layer.
>>
>> Signed-off-by: David Cohen
>> ---
>> arch/arm/plat-omap/include/plat/iommu.h | 7 +++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/include/plat/iommu.h
>> b/arch/arm/plat-omap/include/plat/iommu.h
>> index 69230d6..71f369d 100644
>> --- a/arch/arm/plat-omap/include/plat/iommu.h
>> +++ b/arch/arm/plat-omap/include/plat/iommu.h
>> @@ -109,6 +109,13 @@ struct iommu_platform_data {
>> u32 da_end;
>> };
>>
>> +/* IOMMU errors */
>> +#define OMAP_IOMMU_ERR_TLB_MISS (1 << 0)
>> +#define OMAP_IOMMU_ERR_TRANS_FAULT (1 << 1)
>> +#define OMAP_IOMMU_ERR_EMU_MISS (1 << 2)
>> +#define OMAP_IOMMU_ERR_TBLWALK_FAULT (1 << 3)
>> +#define OMAP_IOMMU_ERR_MULTIHIT_FAULT (1 << 4)
>
> Define these macros in the patch where they are used.
That's fine for me. I can merge patches 1 and 3.
Regards,
David
>
>> +
>> #if defined(CONFIG_ARCH_OMAP1)
>> #error "iommu for this processor not implemented yet"
>> #else
>> --
>> 1.7.1
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 04/10] omap2: Fix camera resources for multiomap
On Mon, Feb 14, 2011 at 3:50 PM, Laurent Pinchart
wrote:
> Hi Felipe,
Hello,
>
> On Monday 14 February 2011 14:41:16 Felipe Balbi wrote:
>> On Mon, Feb 14, 2011 at 02:19:24PM +0100, Laurent Pinchart wrote:
>> > On Monday 14 February 2011 13:35:59 Felipe Balbi wrote:
>> > > On Mon, Feb 14, 2011 at 01:21:31PM +0100, Laurent Pinchart wrote:
>> > > > diff --git a/arch/arm/mach-omap2/devices.c
>> > > > b/arch/arm/mach-omap2/devices.c index 4cf48ea..5d844bd 100644
>> > > > --- a/arch/arm/mach-omap2/devices.c
>> > > > +++ b/arch/arm/mach-omap2/devices.c
>> > > > @@ -38,7 +38,7 @@
>> > > >
>> > > > #if defined(CONFIG_VIDEO_OMAP2) ||
>> > > > defined(CONFIG_VIDEO_OMAP2_MODULE)
>> > > >
>> > > > -static struct resource cam_resources[] = {
>> > > > +static struct resource omap2cam_resources[] = {
>> > >
>> > > should this be __initdata ??
>> >
>> > The resources will be used when the OMAP3 ISP module is loaded. Won't
>> > they be discared if marked as __initdata ?
>>
>> I believe driver core makes a copy of those, no ? not sure.
>
> Not that I know of, but I may be wrong.
I don't think omap2cam_resources would be used at all.
AFAIK, it belongs to omap2xxcam, isn't it? :)
Br,
David
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 03/10] omap3: Add function to register omap3isp platform device structure
On Mon, Feb 14, 2011 at 3:18 PM, Felipe Balbi wrote: > Hi, Hi > > On Mon, Feb 14, 2011 at 02:07:08PM +0100, Laurent Pinchart wrote: >> > > diff --git a/arch/arm/mach-omap2/devices.h >> > > b/arch/arm/mach-omap2/devices.h new file mode 100644 >> > > index 000..12ddb8a >> > > --- /dev/null >> > > +++ b/arch/arm/mach-omap2/devices.h >> > > @@ -0,0 +1,17 @@ >> > > +/* >> > > + * arch/arm/mach-omap2/devices.h >> > > + * >> > > + * OMAP2 platform device setup/initialization >> > > + * >> > > + * This program is free software; you can redistribute it and/or modify >> > > + * it under the terms of the GNU General Public License as published by >> > > + * the Free Software Foundation; either version 2 of the License, or >> > > + * (at your option) any later version. >> > > + */ >> > > + >> > > +#ifndef __ARCH_ARM_MACH_OMAP_DEVICES_H >> > > +#define __ARCH_ARM_MACH_OMAP_DEVICES_H >> > > + >> > > +int omap3_init_camera(void *pdata); >> > >> > missing extern ? >> >> Is that mandatory ? Many (most ?) headers in the kernel don't use the extern >> keyword when declaring functions. > > maybe not mandatory, worth checking what sparse would say though :-p sparse is not complaining. Br, David > > -- > balbi > -- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] OMAP: IOMMU: add missing function declaration
Declaration of exported function 'iopgtable_lookup_entry' is missing from header file. Currently we have a sparse warning as it's not being used externally. Adding its declaration to avoid such warning and allow its usage in future. Signed-off-by: David Cohen --- arch/arm/plat-omap/include/plat/iommu.h |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h index 69230d6..19cbb5e 100644 --- a/arch/arm/plat-omap/include/plat/iommu.h +++ b/arch/arm/plat-omap/include/plat/iommu.h @@ -154,6 +154,8 @@ extern void flush_iotlb_range(struct iommu *obj, u32 start, u32 end); extern void flush_iotlb_all(struct iommu *obj); extern int iopgtable_store_entry(struct iommu *obj, struct iotlb_entry *e); +extern void iopgtable_lookup_entry(struct iommu *obj, u32 da, u32 **ppgd, + u32 **ppte); extern size_t iopgtable_clear_entry(struct iommu *obj, u32 iova); extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end); -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
IOMMU upper layer is already printing error message. OMAP2+ specific
layer may print error message only for debug purpose.
Signed-off-by: David Cohen
---
arch/arm/mach-omap2/iommu2.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index 14ee686..4244a07 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32
*ra)
da = iommu_read_reg(obj, MMU_FAULT_AD);
*ra = da;
- dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
+ dev_dbg(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(KERN_DEBUG "%s ", err_msg[i]);
}
- printk("\n");
+ printk(KERN_DEBUG "\n");
iommu_write_reg(obj, stat, MMU_IRQSTATUS);
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] IOMMU fault callback support
Hi, This patch set adds fault callback support to allow IOMMU users to debug or react when a fault happens. IOMMU faults might be very difficult to reproduce and then to figure out the source of the problem. Currently IOMMU driver prints not so useful debug message and does not notice user about such issue. With a fault callback, IOMMU user may debug much more useful information and/or react to go back to a valid state. Br, David --- David Cohen (2): OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg() OMAP: IOMMU: add support to callback during fault handling arch/arm/mach-omap2/iommu2.c| 27 arch/arm/plat-omap/include/plat/iommu.h | 15 ++- arch/arm/plat-omap/iommu.c | 41 --- 3 files changed, 72 insertions(+), 11 deletions(-) -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] OMAP: IOMMU: add support to callback during fault handling
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
---
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 4244a07..559a066 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(KERN_DEBUG "\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 intrefcount;
struct mutexiommu_lock; /* global for this whole object */
@@ -48,6 +49,7 @@ struct iommu {
struct mutexmmap_lock; /* protect mmap */
int (*isr)(struct iommu *obj);
+ void (*fault_cb)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
void *ctx; /* iommu context: registres saved area */
u32 da_start;
@@ -83,7 +85,7 @@ struct iommu_functions {
int (*enable)(struct iommu *obj);
void (*disable)(struct iommu *obj);
void (*set_twl)(struct iommu *obj, bool on);
- u32 (*fault_isr)(struct iommu *obj, u32 *ra);
+ u32 (*fault_isr)(struct iommu *obj, u32 *ra, u32 *iommu_errs);
void (*tlb_read_cr)(struct iommu *obj, struct cr_regs *cr);
void (*tlb_load_cr)(struct iommu *obj, struct cr_regs *cr);
@@ -109,6 +111,13 @@ struct iommu_platform_data {
u32 da_end;
};
+/* IOMMU errors */
+#define OMAP_IOMMU_ERR_TLB_MISS(1 << 0)
+#define OMAP_IOMMU_ERR_TRANS_FAULT (1 << 1)
+#define OMAP_IOMMU_ERR_EMU_MISS(1 << 2)
+#define OMAP_IOMMU_ERR_TBLWALK_FAULT (1 << 3)
+#define OMAP_IOMMU_ERR_MULTIHIT_FAULT (1 << 4)
+
#if defined(CONFIG_ARCH_OMAP1)
#error "iommu for this processor not implemented yet"
#else
@@ -161,6 +170,10 @@ extern size_t iopgtable_clear_entry(struct iommu *obj, u32
iova);
extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end);
extern struct iommu *iommu_get(const char *name);
extern void iommu_put(struct iommu *obj);
+extern int iommu_set_fault_callback(const char *name,
+ void (*fault_cb)(struct iommu *obj, u32 da,
+u32 errs, void *priv),
+ void *fault_cb_priv);
extern void iommu_save_ctx(struct iommu *obj);
extern void iommu_restore_ctx(struct iommu *obj);
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index b1107c0..7f780ee 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -163,9 +163,9 @@ static u32 get_iopte_attr(struct iotlb_entry *e)
return arch_iommu->get_pte_attr(e);
}
-static u32 iommu_report_fault(struct iommu *obj, u32 *da)
+static u32 iommu_report_fault(struct iommu *obj, u32 *da, u32 *iommu_errs)
{
- return arch_iommu->fault_isr(o
Re: [PATCH 0/2] IOMMU fault callback support
A missing prefix in the cover letter's subject. It's: OMAP: IOMMU: Br, David On Tue, Feb 15, 2011 at 3:20 PM, David Cohen wrote: > Hi, > > This patch set adds fault callback support to allow IOMMU users to debug or > react when a fault happens. > IOMMU faults might be very difficult to reproduce and then to figure out > the source of the problem. Currently IOMMU driver prints not so useful > debug message and does not notice user about such issue. > With a fault callback, IOMMU user may debug much more useful information > and/or react to go back to a valid state. > > Br, > > David > --- > > David Cohen (2): > OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg() > OMAP: IOMMU: add support to callback during fault handling > > arch/arm/mach-omap2/iommu2.c | 27 > arch/arm/plat-omap/include/plat/iommu.h | 15 ++- > arch/arm/plat-omap/iommu.c | 41 > --- > 3 files changed, 72 insertions(+), 11 deletions(-) > > -- > 1.7.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
On Tue, Feb 15, 2011 at 3:38 PM, Sergei Shtylyov wrote:
> Hello.
Hi,
>
> On 15-02-2011 16:20, David Cohen wrote:
>
>> IOMMU upper layer is already printing error message. OMAP2+ specific
>> layer may print error message only for debug purpose.
>
>> Signed-off-by: David Cohen
>> ---
>> arch/arm/mach-omap2/iommu2.c | 6 +++---
>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>> index 14ee686..4244a07 100644
>> --- a/arch/arm/mach-omap2/iommu2.c
>> +++ b/arch/arm/mach-omap2/iommu2.c
>> @@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj,
>> u32 *ra)
>> da = iommu_read_reg(obj, MMU_FAULT_AD);
>> *ra = da;
>>
>> - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
>> + dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);
>
> Note that dev_dbg() will only print something if either DEBUG or
> CONFIG_DYNAMIC_DEBUG are defined...
That's my plan.
>
>>
>> for (i = 0; i< ARRAY_SIZE(err_msg); i++) {
>> if (stat & (1<< i))
>> - printk("%s ", err_msg[i]);
>> + printk(KERN_DEBUG "%s ", err_msg[i]);
>
> ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug()
> instead.
>
>> }
>> - printk("\n");
>> + printk(KERN_DEBUG "\n");
>
> Here too... Although wait, it should be KERN_CONT instead! Debug levels
> are only attributed to the whole lines.
But your observation is correct. I'll resend it with KERN_CONT instead.
Regards,
David
>
> WBR, Sergei
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
On Tue, Feb 15, 2011 at 3:56 PM, Sergei Shtylyov wrote:
> On 15-02-2011 16:44, David Cohen wrote:
>
>>>> IOMMU upper layer is already printing error message. OMAP2+ specific
>>>> layer may print error message only for debug purpose.
>
>>>> Signed-off-by: David Cohen
>>>> ---
>>>> arch/arm/mach-omap2/iommu2.c | 6 +++---
>>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>
>>>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>>>> index 14ee686..4244a07 100644
>>>> --- a/arch/arm/mach-omap2/iommu2.c
>>>> +++ b/arch/arm/mach-omap2/iommu2.c
>>>> @@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu
>>>> *obj,
>>>> u32 *ra)
>>>> da = iommu_read_reg(obj, MMU_FAULT_AD);
>>>> *ra = da;
>>>>
>>>> - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
>>>> + dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da);
>
>>> Note that dev_dbg() will only print something if either DEBUG or
>>> CONFIG_DYNAMIC_DEBUG are defined...
>
>> That's my plan.
>
>>>> for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
>>>> if (stat& (1<< i))
>>>> - printk("%s ", err_msg[i]);
>>>> + printk(KERN_DEBUG "%s ", err_msg[i]);
>
>>> ... unlike printk(KERN_DEBUG...). You probably want to use pr_debug()
>>> instead.
>
>>>> }
>>>> - printk("\n");
>>>> + printk(KERN_DEBUG "\n");
>
>>> Here too... Although wait, it should be KERN_CONT instead! Debug levels
>>> are only attributed to the whole lines.
>
>> But your observation is correct. I'll resend it with KERN_CONT instead.
>
> This won't play out correctly anyway. If DEBUG is not #define'd, the
> beginning of line won't be printed but the continuations will. You just
> can't do it correctly with dev_dbg(), unless you break the single line into
> several ones.
Yes, I got this situation. I'm coming with a proper solution on next version.
Br,
David
>
>> Regards,
>
>> David
>
> WBR, Sergei
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] OMAP2+: IOMMU: change OMAP2+ error message to dev_dbg()
On Tue, Feb 15, 2011 at 3:59 PM, Jarkko Nikula wrote: > On Tue, 15 Feb 2011 15:44:27 +0200 > David Cohen wrote: > >> >> @@ -163,13 +163,13 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, >> >> u32 *ra) >> >> da = iommu_read_reg(obj, MMU_FAULT_AD); >> >> *ra = da; >> >> >> >> - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da); >> >> + dev_dbg(obj->dev, "%s:\tda:%08x ", __func__, da); >> > >> > Note that dev_dbg() will only print something if either DEBUG or >> > CONFIG_DYNAMIC_DEBUG are defined... >> >> That's my plan. >> > So it's sure that a developer won't need these error dumps when > receiving an error report? I.e. IOMMU upper level errors give enough > information to start doing own debugging? Yes, developers do need this information. But it's a bit useless tell only we've got an iommu fault, due to many places might be causing it. My purpose is to let the debug responsibility to IOMMU users. They have access to the iovmm layer as well and can provide a much more useful information. e.g. OMAP3 ISP has many submodules using IOMMU. With a fault callback, it can dump all the iovm areas and the faulty 'da' too. It might indicate which submodule was responsible for the issue. Of course we can just let this debug messages the way they are and print this redundant information. But IMO it's not necessary. Regards, David > > Just my 2 cents. > > -- > Jarkko > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/1] OMAP: IOMMU fault callback support
Hi, v2: Previous patch 1/2 on v1 was dropped. --- v1: This patch set adds fault callback support to allow IOMMU users to debug or react when a fault happens. IOMMU faults might be very difficult to reproduce and then to figure out the source of the problem. Currently IOMMU driver prints not so useful debug message and does not notice user about such issue. With a fault callback, IOMMU user may debug much more useful information and/or react to go back to a valid state. Br, David --- David Cohen (1): OMAP: IOMMU: add support to callback during fault handling 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(-) -- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
