Re: [PATCH v2 00/15] tidspbridge driver MMU-related cleanups
Hi Laurent, On Wed, Sep 19, 2012 at 7:06 AM, Laurent Pinchart wrote: > Hello, > > Here's the second version of my tidspbridge MMU-related cleanup patches. The > first version has been sent privately only, don't try to search the mailing > list archive for it :-) > > Replacing hw/hw_mmu.c and part of core/tiomap3430.c with generic IOMMU calls > should be less difficult now. Anyone would like to give it a try? > > Laurent Pinchart (14): > tidspbridge: hw_mmu: Reorder functions to avoid forward declarations > tidspbridge: hw_mmu: Removed unused functions > tidspbridge: tiomap3430: Reorder functions to avoid forward > declarations > tidspbridge: tiomap3430: Remove unneeded dev_context local variables > tidspbridge: tiomap3430: Factor out common page release code > tidspbridge: tiomap3430: Remove ul_ prefix > tidspbridge: tiomap3430: Remove unneeded local variables > tidspbridge: Fix VM_PFNMAP mapping > tidspbridge: Remove unused hw_mmu_map_attrs_t::donotlockmpupage field > arm: omap: iommu: Include required headers in iommu.h and iopgtable.h > tidspbridge: Use constants defined in IOMMU platform headers > tidspbridge: Simplify pte_update and mem_map_vmalloc functions > tidspbridge: Use correct types to describe physical, MPU, DSP > addresses > tidspbridge: Replace hw_mmu_map_attrs_t structure with a prot > bitfield Thanks, tested on beagle-xM, they look good! Can you submit them to Greg KH and de...@driverdev.osuosl.org, preferably with a 'staging:' prefix along with the current subject. The only thing of concern is that: ARM: OMAP: iommu: fix including iommu.h without IOMMU_API selected Might be taking a different path than these to mainline[1]. Cheers, Omar --- [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg76319.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 4/8] ARM: OMAP2+: hwmod: revise hardreset behavior
Hi Paul, On Thu, Apr 19, 2012 at 2:46 PM, Paul Walmsley wrote: > Hi Benoît, > > On Thu, 19 Apr 2012, Cousson, Benoit wrote: > >> On 4/19/2012 7:17 PM, Paul Walmsley wrote: >> > On Thu, 19 Apr 2012, Cousson, Benoit wrote: >> > >> > > The concern of the people doing SW in these embedded processors was >> > > mainly >> > > because we were releasing the reset of processor without loading any SW >> > > first >> > > and thus the processor was executing some random instructions. >> > > >> > > So if we consider a better hwmods definition, we can potentially fix the >> > > MMU >> > > case: >> > > >> > > 'mmu_ipu': >> > > 'rst_ipu_mmu_cache' >> > > >> > > 'mmu_dsp': >> > > 'rst_dsp_mmu_cache' >> > > >> > > 'iva_config': >> > > 'rst_logic' >> > >> > Wouldn't these still be "pseudo-hwmods?" Or do each of these actually >> > have address space, interconnect ports, etc.? >> >> Yes, these ones are the only real IPs for MPU stand point, with interconnect >> port and configuration registers. > > These are the MMUs documented in Chapter 20 of the OMAP4430 TRM > vX, right? > > I don't really understand the interaction of the hardreset lines with > these IPs. Chapter 20 doesn't seem to mention the PRCM-controllable > hardreset lines at all. It only mentions the OCP_SYSCONFIG registers > associated with them. > > Meanwhile Table 3-375 "RM_DSP_RSTCTRL" mentions a DSP MMU, cache, and > slave interface reset line. Is that referring to the same MMU that is > mentioned in Chapter 20? The end of Section 20.2 "MMU Integration" > mentions two different DSP MMUs, a "shared cache MMU" and an "L2 MMU" - > maybe the hardreset line only pertains to the first MMU? I don't thinks so, because trying to read the L2 MMU registers with DSP RST2 asserted causes an L3 error. DSP's "shared cache MMU" refers to section 5.3.2.5 Attribute MMU which is inside the dsp megamodule, OTOH "L2 MMU" refers to section 5.3.4 MMU, the latter is the one being referenced in section 20. >> > > But then we do have the processor resets that have to be exposed >> > > somewhere. >> > > >> > > 'ipu': >> > > 'rst_cpu0' >> > > 'rst_cpu1' >> > > >> > > 'dsp': >> > > 'rst_dsp' >> > > >> > > 'iva': >> > > 'rst_seq1' >> > > 'rst_seq2' >> > > >> > > None of these one should be controlled automatically. >> > >> > Don't we still want to put these IP blocks into reset until a driver for >> > these IP blocks is loaded? >> >> Yes, indeed, my point is where are we going to declare these reset lines if >> we >> do not have any fake hwmods to store them. > > Wouldn't we associate them with the processor hwmods? e.g., > omap44xx_iva_hwmod, omap44xx_dsp_hwmod ? > >> Well, for the MMU we can, these are regular IPs (almost). The real issues was >> for the processors. > > Omar, do you know how these hardreset lines interact with the MMUs > described in the TRM Chapter 20? For M3, RM_MPU_M3_RSTCTRL[2].RST3 releases both the cache and mmu interfaces, this one is needed if you want to program the mmu by touching any of the MMU registers, typical initialization is to deassert this line and then enable the M3 clock (since it is shared with its MMU) which should allow the reset to complete *only* after the clock has been enabled. For DSP, RM_DSP_RSTCTRL[1].RST2 it also releases dsp, cache and mmu (however the dsp doesn't boot until RST1 is deasserted), it is the same rationale as above; you deassert RST2, enable dsp clock (so reset can complete) and then program mmu. Touching any mmu registers without deasserting its reset line (even if the associated clock is ON) generates a L3 error. This can be found in 3.5.6 Reset sequences for DSP and M3. 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 v3 4/4] ARM: omap: pass minimal SoC/board data for UART from dt
On Wed, Apr 11, 2012 at 3:39 AM, Cousson, Benoit wrote: > Yes, it is a known issue and the fix was unfortunately was not merged during > -rc phases but is fixed in 3.4 and should be fixed in 3.3 stable branch as > well. I received the notification from Greg KH 2 weeks ago about the stable: > Patch "tty: serial: OMAP: Fix oops due to NULL pdata in DT boot" has been > added to the 3.3-stable tree > > You should just switch to 3.4-rc2 or get the patch if you are stuck to 3.3. Thanks, switching to 3.4-rc kernels fixed the problem (for cramfs), and right now this works for me. 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 v3 4/4] ARM: omap: pass minimal SoC/board data for UART from dt
Hi,
On Wed, Dec 14, 2011 at 5:55 AM, Rajendra Nayak wrote:
> Pass minimal data needed for console boot, from dt, for
> OMAP4 panda/sdp and OMAP3 beagle boards, and get rid of the
> static initialization from generic board file.
...
> diff --git a/arch/arm/mach-omap2/board-generic.c
> b/arch/arm/mach-omap2/board-generic.c
> index 63b5416..a508ed5 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -69,7 +69,6 @@ static void __init omap_generic_init(void)
> if (node)
> irq_domain_add_simple(node, 0);
>
> - omap_serial_init();
> omap_sdrc_init(NULL, NULL);
>
> of_platform_populate(NULL, omap_dt_match_table, NULL, NULL);
> --
> 1.7.1
I'm fairly new to DT and I'm trying to boot it with pandaboard and
k3.3, however above hunk deletes omap serial initialization, which
causes a panic on boot, because pdata is NULL:
static void serial_omap_start_tx(struct uart_port *port)
{
...
if (pdata->set_noidle)
Perhaps because this change skips the following path:
omap_serial_init->omap_serial_board_init->omap_serial_init_port
Where pdata is built in omap_device_build.
I'm just trying to confirm that I'm not alone or doing some silly
thing before getting in depth with the code.
Here it is the panic:
[2.024688] Unable to handle kernel NULL pointer dereference at
virtual address 0028
[2.031005] pgd = ed6f
[2.036315] [0028] *pgd=af339831, *pte=, *ppte=
[2.042907] Internal error: Oops: 17 [#1] SMP
[2.046630] Modules linked in:
[2.046630] CPU: 0Not tainted (3.3.0-2-gda19af1-dirty #11)
[2.046630] PC is at serial_omap_start_tx+0x1cc/0x218
[2.046630] LR is at serial_omap_start_tx+0x1b8/0x218
[2.067840] pc : []lr : []psr: 6193
[2.067840] sp : c21cbe70 ip : 0001 fp : a113
[2.073699] r10: ef107000 r9 : ed684600 r8 : 001c
[2.085327] r7 : 0002 r6 : 0007 r5 : r4 : ed684600
[2.086029] r3 : ef1281c0 r2 : fa02 r1 : 0004 r0 : c02b3a40
[2.086029] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment user
[2.101684] Control: 10c53c7d Table: ad6f004a DAC: 0015
[2.101684] Process S10udev (pid: 497, stack limit = 0xc21ca2f8)
[2.116973] Stack: (0xc21cbe70 to 0xc21cc000)
[2.123443] be60: a113
c0475b74 0002
[2.126007] be80: c02ac7e0 ef107000 ed684600 2113
c02ac830 ed690ad0
[2.139099] bea0: ed6e981c c02ae348 c02ae288 ef107000 c21ca000
001c ed6e9800 ef1074fc
[2.145385] bec0: c049cab4 ef1dcd80 ed6e9800 c0296aa8 6113
c02925e8 ef107194 ef107120
[2.152191] bee0: ef0003c0 ef1281c0 c0076214 ef1071b4
ef1071b4 ef1074b4 ef1dcd80
[2.159790] bf00: ef107000 b6f5c000 001c c21ca000 0400
c02968c8 c0292638
[2.159790] bf20: c06e3ea0 001c c02968c8 ef168b80 c21cbf80
ef1281c0 ef1dcd80
[2.175811] bf40: 001c b6f5c000 c21cbf80
c010841c
[2.188476] bf60: c0014400 ef1281c0 ef1dcd80 b6f5c000 001c
0004 c0108570
[2.195068] bf80: 001c 001c
b6f345c8 001c c00144a8
[2.195068] bfa0: c21ca000 c00142e0 001c b6f345c8 0001
b6f5c000 001c
[2.210723] bfc0: 001c b6f345c8 001c 0004 b6f5c000
0001
[2.210723] bfe0: be988948 b6e5ae98 b6eb7adc 6110
0001 89389af9 fef855e7
[2.226379] [] (serial_omap_start_tx+0x1cc/0x218) from
[] (uart_start+0x68/0x6c)
[2.241973] [] (uart_start+0x68/0x6c) from []
(uart_write+0xc0/0xe4)
[2.241973] [] (uart_write+0xc0/0xe4) from []
(n_tty_write+0x1e0/0x42c)
[2.257629] [] (n_tty_write+0x1e0/0x42c) from
[] (tty_write+0x140/0x23c)
[2.270446] [] (tty_write+0x140/0x23c) from []
(vfs_write+0xb0/0x134)
[2.278594] [] (vfs_write+0xb0/0x134) from []
(sys_write+0x40/0x70)
[2.281311] [] (sys_write+0x40/0x70) from []
(ret_fast_syscall+0x0/0x3c)
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 5/8] ARM: OMAP2+: hwmod: ensure that SYSCONFIG bits are reprogrammed after a reset
On Thu, Mar 15, 2012 at 1:25 AM, Paul Walmsley wrote: > Hola Omar, Hola :) > On Wed, 14 Mar 2012, Ramirez Luna, Omar wrote: > >> If we reached here the reset lines will be asserted, and then the code >> below touches sysc registers on a module under reset. > > Do you know of any case where this would be a problem? Seems like it > would only affect IP blocks that have both hard reset lines and SYSCONFIG > registers, and as far as I know, we don't have any of those defined? MMU (not yet mainlined) has both, a reset line and a sysconfig. I have been holding the hwmod for some time, but now that rpmsg/remoteproc is going to mainline it could make use of it along with omap3isp, however now I need to define functions to handle the reset lines (although I was fine with hwmod handling it). AFAIKnew, hwmod handling the reset line was fine (IMHO), the only 2 things were: - For the drivers to somehow make use of shutdown/enable if they needed they hwmod under reset and out. - The annoying: hwmod XX failed to hardreset because of the wrong reset sequence but causing no functional issues. 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 5/8] ARM: OMAP2+: hwmod: ensure that SYSCONFIG bits are reprogrammed after a reset
Hi Paul,
2012/2/27 Paul Walmsley :
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c
> b/arch/arm/mach-omap2/omap_hwmod.c
> index db4ad41..aeb6f4c 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1490,13 +1490,22 @@ static int _reset(struct omap_hwmod *oh)
> pr_debug("omap_hwmod: %s: resetting\n", oh->name);
>
> if (oh->class->reset)
> - return oh->class->reset(oh);
> -
> - if (!oh->rst_lines_cnt)
> - return _ocp_softreset(oh);
> + oh->class->reset(oh);
> + else if (!oh->rst_lines_cnt)
> + _ocp_softreset(oh);
> + else
> + for (i = 0; i < oh->rst_lines_cnt; i++)
> + _assert_hardreset(oh, oh->rst_lines[i].name);
If we reached here the reset lines will be asserted, and then the code
below touches sysc registers on a module under reset.
This would crash on _setup->_setup_reset->_reset.
Adding a 'return 0' I believe fixes the behavior, boots the board and
leaves the hwmod under reset.
- else
+ } else {
for (i = 0; i < oh->rst_lines_cnt; i++)
_assert_hardreset(oh, oh->rst_lines[i].name);
+ return 0;
+ }
>
> - for (i = 0; i < oh->rst_lines_cnt; i++)
> - _assert_hardreset(oh, oh->rst_lines[i].name);
> + /*
> + * OCP_SYSCONFIG bits need to be reprogrammed after a
> + * softreset. The _enable() function should be split to avoid
> + * the rewrite of the OCP_SYSCONFIG register.
> + */
> + if (oh->class->sysc) {
> + _update_sysc_cache(oh);
> + _enable_sysc(oh);
> + }
Best 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] ARM: OMAP2: fix mailbox init code
Hi,
On Thu, Feb 23, 2012 at 3:18 AM, Bedia, Vaibhav wrote:
> On Thu, Feb 23, 2012 at 14:23:35, Ohad Ben-Cohen wrote:
> [...]
>>
>> Which happens on CONFIG_ARCH_OMAP2 && !CONFIG_SOC_OMAP2420, due to
>> missing omap2_mboxes declaration.
>>
> [...]
>>
>> -struct omap_mbox *omap2_mboxes[] = { &mbox_dsp_info, &mbox_iva_info, NULL };
>> +#ifdef CONFIG_ARCH_OMAP2
>> +struct omap_mbox *omap2_mboxes[] = {
>> + &mbox_dsp_info,
>> +#ifdef CONFIG_SOC_OMAP2420
>> + &mbox_iva_info,
>> +#endif
>> + NULL
>> +};
>> #endif
>>
>> #if defined(CONFIG_ARCH_OMAP4)
>
> Instead of adding more #ifs can they be completely removed please?
I'll rebase/repost this series:
[PATCH 0/7] OMAP: mailbox: removing static declarations
http://comments.gmane.org/gmane.linux.ports.arm.omap/59620
In the meantime I would appreciate comments.
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: Latest OMAP randconfig build error
On Wed, Feb 22, 2012 at 11:58 AM, Tony Lindgren wrote:
> * Ohad Ben-Cohen [120222 01:30]:
>> + Tony, Suman
>>
>> On Wed, Feb 22, 2012 at 10:51 AM, Russell King - ARM Linux
>> wrote:
>> > arch/arm/mach-omap2/mailbox.c: In function 'omap2_mbox_probe':
>> > arch/arm/mach-omap2/mailbox.c:354: error: 'omap2_mboxes' undeclared (first
>> > use in this function)
>> > arch/arm/mach-omap2/mailbox.c:354: error: (Each undeclared identifier is
>> > reported only once
>> > arch/arm/mach-omap2/mailbox.c:354: error: for each function it appears in.)
>>
>> The below should trivially solve this, but I wonder if there was any
>> other merit in explicitly using CONFIG_SOC_OMAP2420 there (any
>> different between 2420 and 2430 in that respect ?).
>>
>> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
>> index 609ea2d..e61d275 100644
>> --- a/arch/arm/mach-omap2/mailbox.c
>> +++ b/arch/arm/mach-omap2/mailbox.c
>> @@ -258,7 +258,7 @@ struct omap_mbox mbox_dsp_info = {
>> struct omap_mbox *omap3_mboxes[] = { &mbox_dsp_info, NULL };
>> #endif
>>
>> -#if defined(CONFIG_SOC_OMAP2420)
>> +#if defined(CONFIG_ARCH_OMAP2)
>> /* IVA */
>> static struct omap_mbox2_priv omap2_mbox_iva_priv = {
>> .tx_fifo = {
>
> 2430 is like omap3 for the mailbox. So the code we have seems
> wrong trying to initialize it like 2420 mailbox. So we either
> need a new entry for omap2430_mboxes[], or should just bail
> out from the probe for 2430 for the fix.
Yes, current code tries to configure both mboxes in a 2430, however it
shouldn't be assigning an irq line for the iva mbox, and any request
for iva mbox should fail due to that.
Code is wrong to register both in 2430 though.
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] staging: tidspbridge: enable watchdog by default
On Tue, Feb 14, 2012 at 10:23 AM, Felipe Contreras wrote: >> When that case is applicable, we should first modify the loader code >> or prepare the baseimages to be common so we can get rid of specific >> loaders and just dump them into memory. > > I'd say the less workarounds, the better. If there are ever more base images compatible with the dsp, I would say that unifying them into a common format to be dumped in memory isn't a workaround, and in that process we can get rid of the custom loader code. >> WDT could be detected by prepending common symbols into the baseimages >> or just making the new images to treat all peripherals as resources, >> that is, the new baseimg should actually request the wdt3 as any other >> clock. > > I see, but if wdt3 is requested as another clock, the Linux ARM side > would still need to know how to threat the WDT. Tidspbridge does know how to treat other clock request from dsp (gpt, mcbsp), it would be a matter of adding the logic in the arm side for any dsp image that is able to do it, however this doesn't apply to the current (latest) base image since it assumes the wdt3 is always controlled by tidspbridge. 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] staging: tidspbridge: enable watchdog by default
On Thu, Feb 9, 2012 at 9:18 PM, Greg KH wrote: > On Thu, Feb 09, 2012 at 04:45:00PM -0800, Ramirez Luna, Omar wrote: >> Hi, >> >> On Thu, Feb 9, 2012 at 3:30 PM, Felipe Contreras >> wrote: >> >> Again, I'm totally confused as to _WHY_ this needs to be y. What is >> >> causing this oops without it? If an oops is happening, then shouldn't >> >> this be a strict dependancy? Why allow it to be disabled at all if it >> >> can break your box if you don't enable it? >> > >> > It's not an oops, it's a warning, and again, it depends on the >> > firmware being used. We don't have control over that, and we have no >> > way to detect if this feature is there. It's up to the user. >> >> I have been thinking more into it, how about looking for a WDT symbol >> inside the baseimage to decide whether to turn ON/OFF WDT3, this would >> mean that the code is always compiled in, but the decision to turn it >> on/off is made at runtime. > > I totally don't understand, why not just silence the warning properly > then? > > I fail to understand why this warning happens, why it depends on the > firmware, and why you can't detect it at runtime to not do it. And how > it all ties into a kconfig option... Just FTR, the warning comes from an interconnect driver that reports errors. This specific warning occurs because the dsp tries to access wdt3 registers without a clock, hence turning ON the wdt3 (or default y for wdt3 Kconfig) will make the warning to disappear when first loading a base image into the dsp, because now there will be a clock for the registers. It depends on the firmware because the accesses are made from the dsp itself. 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] staging: tidspbridge: enable watchdog by default
On Feb 11, 2012 3:03 PM, "Felipe Contreras" wrote: > > On Sat, Feb 11, 2012 at 9:19 PM, Ramirez Luna, Omar > wrote: > > > > On Feb 10, 2012 12:44 PM, "Felipe Contreras" > > wrote: > >> > >> On Fri, Feb 10, 2012 at 10:35 PM, Dan Carpenter > >> wrote: > >> > On Fri, Feb 10, 2012 at 09:42:32PM +0200, Felipe Contreras wrote: > >> >> On Fri, Feb 10, 2012 at 8:00 PM, Dan Carpenter > >> >> wrote: > >> >> > On Fri, Feb 10, 2012 at 01:30:48AM +0200, Felipe Contreras wrote: > >> >> >> It's not an oops, it's a warning, and again, it depends on the > >> >> >> firmware being used. We don't have control over that, and we have no > >> >> >> way to detect if this feature is there. It's up to the user. > >> >> > > >> >> > Perhaps just remove the warning message and handle the condition > >> >> > instead of printing a stack dump? The user should be triggering > >> >> > stack dumps. What on earth? > >> >> > >> >> The warning doesn't come from the driver. > >> > > >> > I'm not sure I understand. Are you saying that because it comes > >> > from the arch/ directory, it can't be fixed? I have good news for > >> > you my friend. :) It's all open source! \o/ > >> > >> The fact that you _can_ remove the warning doesn't mean you should. To > >> me it sounds like a proper warning. > >> > >> > Anyway, I saw in another email that Omar is working on a fix so > >> > probably we can just wait for his patch, yes? > >> > >> He only proposed a solution, I doubt he is working on. And to me, that > >> sounded like a hack rather than a proper fix. > > > > I'm out on travel but will be able to look at it on Monday. > > > > Well, I think it is the right way, you look on the firmware if it has WDT > > you use it if it doesn't then you don't. Rather than guessing if you have > > the feature. It would be like reading a config option in the firmware. > > Yeah, but it's not really firmware, it's an operating system image. I > can be running Linux there, and I might have implemented WDT. How is > that code going to find that out? Tidspbridge loader doesn't support that use case, baseimage and let's say a dsp-linuximg won't have the same layout, hence it won't be able to parse and load the latter. When that case is applicable, we should first modify the loader code or prepare the baseimages to be common so we can get rid of specific loaders and just dump them into memory. WDT could be detected by prepending common symbols into the baseimages or just making the new images to treat all peripherals as resources, that is, the new baseimg should actually request the wdt3 as any other clock. 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] staging: tidspbridge: enable watchdog by default
Hi, On Thu, Feb 9, 2012 at 3:30 PM, Felipe Contreras wrote: >> Again, I'm totally confused as to _WHY_ this needs to be y. What is >> causing this oops without it? If an oops is happening, then shouldn't >> this be a strict dependancy? Why allow it to be disabled at all if it >> can break your box if you don't enable it? > > It's not an oops, it's a warning, and again, it depends on the > firmware being used. We don't have control over that, and we have no > way to detect if this feature is there. It's up to the user. I have been thinking more into it, how about looking for a WDT symbol inside the baseimage to decide whether to turn ON/OFF WDT3, this would mean that the code is always compiled in, but the decision to turn it on/off is made at runtime. 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] staging: tidspbridge: enable watchdog by default
On Wed, Feb 1, 2012 at 2:11 AM, Felipe Contreras wrote: > On Wed, Feb 1, 2012 at 5:22 AM, Justin P. Mattock > wrote: >> not sure if I see this warning or not on my machine, but is there a fix for >> the warning? I would rather see that, than hide it with setting: default y >> (if that's what its doing!) > > No, there's no fix. IIRC Omar said it would not be easy. Indeed, the ideal would be the ability to turn off the config option but on the dsp side (when compiling the baseimage), however the release package only contains binaries given that the source is not open. I have been exploring introducing a flag in the shared memory area which would require a new baseimage release. 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 2/2] staging: tidspbridge: fix incorrect free to drv_datap
On Tue, Jan 31, 2012 at 2:21 AM, Dan Carpenter wrote: > On Mon, Jan 30, 2012 at 07:20:18PM -0600, Omar Ramirez Luna wrote: >> This structure is still used after it has been freed, since it >> is being allocated in probe, calls to free it have been moved to >> module's remove routine. >> >> This should fix the follwoing messages when attempting to remove the >> module: >> drv_get_first_dev_extension: Failed to retrieve the object handle >> drv_get_first_dev_extension: Failed to retrieve the object handle >> drv_destroy: Failed to store DRV object >> mgr_destroy: Failed to store MGR object >> > > So this is only triggered when you do an rmmod to remove the module? Yes. > Probably that's not stable material. The critical issue is that for a small window the freed memory can be filled with something else and the driver still might dereference that memory which no longer belongs to it, thus causing a crash. But I guess that falls into "this can be a problem", it is ok if it is being left out of stable. Thanks, 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] tidspbridge: Rename module from bridgedriver to tidspbridge
On Tue, Jan 24, 2012 at 3:25 PM, Joe Perches wrote: > tidspbridge when built as a module is named bridgedriver. > > bridgedriver is not a particularly good module name. > > tidspbridge is what the source is named. That seems > a more appropriate module name too as it describes > the hardware function better. > > Signed-off-by: Joe Perches > --- > drivers/staging/tidspbridge/Makefile | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/tidspbridge/Makefile > b/drivers/staging/tidspbridge/Makefile > index fd6a276..8c8c92a 100644 > --- a/drivers/staging/tidspbridge/Makefile > +++ b/drivers/staging/tidspbridge/Makefile > @@ -1,4 +1,4 @@ > -obj-$(CONFIG_TIDSPBRIDGE) += bridgedriver.o > +obj-$(CONFIG_TIDSPBRIDGE) += tidspbridge.o > > libgen = gen/gh.o gen/uuidutil.o > libcore = core/chnl_sm.o core/msg_sm.o core/io_sm.o core/tiomap3430.o \ > @@ -13,7 +13,7 @@ libdload = dynload/cload.o dynload/getsection.o > dynload/reloc.o \ > dynload/tramp.o > libhw = hw/hw_mmu.o > > -bridgedriver-y := $(libgen) $(libservices) $(libcore) $(libpmgr) $(librmgr) \ > +tidspbridge-y := $(libgen) $(libservices) $(libcore) $(libpmgr) $(librmgr) \ > $(libdload) $(libhw) > > #Machine dependent > > Sound good to me, and unless there is an objection, it also gives plenty of time for the change to be noticed. 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 4/5] staging: tidspbridge: silence the compiler
2012/1/23 Víctor Manuel Jáquez Leal : > Silence the warning when compiling drv_interface.c > > Signed-off-by: Víctor Manuel Jáquez Leal > --- > drivers/staging/tidspbridge/rmgr/drv_interface.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c > b/drivers/staging/tidspbridge/rmgr/drv_interface.c > index c7015f5..3bbe443 100644 > --- a/drivers/staging/tidspbridge/rmgr/drv_interface.c > +++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c > @@ -273,8 +273,9 @@ static int bridge_mmap(struct file *filp, struct > vm_area_struct *vma) > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > dev_dbg(bridge, "%s: vm filp %p offset %x start %lx end %lx page_prot " > - "%lx flags %lx\n", __func__, filp, offset, > - vma->vm_start, vma->vm_end, vma->vm_page_prot, vma->vm_flags); > + "%ulx flags %lx\n", __func__, filp, offset, > + vma->vm_start, vma->vm_end, vma->vm_page_prot, > + vma->vm_flags); > > status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, > vma->vm_end - vma->vm_start, > -- > 1.7.8.3 > Looks good. 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 3/5] staging: tidspbridge: Lindent to drv_interface.c
2012/1/23 Víctor Manuel Jáquez Leal :
> No functional changes.
>
> According to Lindent, the file drv_internface.c had some lines with bad
> indentation.
>
> This commit is the output of Lindent.
Usually lindent tends to do whatever it wants, unless carefully configured...
...
> @@ -342,9 +342,10 @@ static void bridge_recover(struct work_struct *work)
> if (atomic_read(&bridge_cref)) {
> INIT_COMPLETION(bridge_comp);
> while (!wait_for_completion_timeout(&bridge_comp,
> -
> msecs_to_jiffies(REC_TIMEOUT)))
> - pr_info("%s:%d handle(s) still opened\n",
> - __func__, atomic_read(&bridge_cref));
> + msecs_to_jiffies
> + (REC_TIMEOUT)))
Like here, it just split msecs_to_jiffies(REC_TIMEOUT) into 2 lines
making it a little harder to read.
> + pr_info("%s:%d handle(s) still opened\n", __func__,
> + atomic_read(&bridge_cref));
I remember the rule was to break lines as far to the right as
possible, no? Chapter 2 CodingStyle, same for the other similar
changes.
...
> @@ -547,10 +548,9 @@ static int __devexit omap34_xx_bridge_remove(struct
> platform_device *pdev)
> pr_err("%s: Failed to retrieve the object handle\n", __func__);
> goto func_cont;
> }
> -
Blank line removed?
> #ifdef CONFIG_TIDSPBRIDGE_DVFS
> if (cpufreq_unregister_notifier(&iva_clk_notifier,
> - CPUFREQ_TRANSITION_NOTIFIER))
> + CPUFREQ_TRANSITION_NOTIFIER))
> pr_err("%s: cpufreq_unregister_notifier failed for iva2_ck\n",
> __func__);
> #endif /* #ifdef CONFIG_TIDSPBRIDGE_DVFS */
> --
> 1.7.8.3
>
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 2/5] staging: tidspbridge: remove unused header
2012/1/24 Felipe Contreras : > 2012/1/23 Víctor Manuel Jáquez Leal : >> No functional changes. >> >> The header file drv_interface.h was only used locally, hence there's no need >> to have it. >> >> Also the only prototyped functions were the file_operations callbacks, then >> this commit moves them up to avoid prototyping too. ... > > But looks good to me either way. FWIW, I agree. 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 1/5] staging: tidspbridge: more readable code
2012/1/24 Felipe Contreras : > 2012/1/23 Víctor Manuel Jáquez Leal : >> Uppercase function names are not pretty. Also the code flow readability is >> enhanced. > > Looks good to me. FWIW, I agree. 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: 3.3-rc1 console lag (was: Re: [PATCH v8 00/20] OMAP2+: UART: Runtime adaptation + cleanup)
Hi Paul, On Wed, Jan 25, 2012 at 9:00 PM, Paul Walmsley wrote: ... >> > Ensure CONFIG_OMAP_PRM is set while testing irq_chaining with uart. >> > And for pm_qos usage ensure CONFIG_CPU_IDLE is selected other wise >> > console might be sluggish. >> >> There is console lag for omap2plus_defconfig given that >> CONFIG_CPU_IDLE is not enabled. Is the intention to force CPU_IDLE >> into the defconfig or find an alternative for the new pm_qos when cpu >> idle is disabled. >> >> Seen on beagle-xm and 3.3-rc1. > > Try this > > http://marc.info/?l=linux-arm-kernel&m=132754676814391&w=2 I tried the series and the console returned to normal, I can confirm that the following patch helps: tty: serial: OMAP: use a 1-byte RX FIFO threshold in PIO mode Thanks, 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
3.3-rc1 console lag (was: Re: [PATCH v8 00/20] OMAP2+: UART: Runtime adaptation + cleanup)
Hi, On Fri, Nov 11, 2011 at 3:57 AM, Govindraj.R wrote: > Converting uart driver to adapt to pm runtime API's. > Code re-org + cleanup. > Moving some functionality from serial.c to omap-serial.c ... > > Ensure CONFIG_OMAP_PRM is set while testing irq_chaining with uart. > And for pm_qos usage ensure CONFIG_CPU_IDLE is selected other wise > console might be sluggish. There is console lag for omap2plus_defconfig given that CONFIG_CPU_IDLE is not enabled. Is the intention to force CPU_IDLE into the defconfig or find an alternative for the new pm_qos when cpu idle is disabled. Seen on beagle-xm and 3.3-rc1. 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: Bad page state in tidspbridge driver
Hi Laurent, On Sun, Jan 8, 2012 at 12:17 PM, Laurent Pinchart wrote: > Hi, > > I'm hitting what seems to be a tidspbridge driver issue when using gst-dsp > with the DSP JPEG encoder. > > My application captures images from the OMAP3 ISP directly to frame buffer > memory for display. It then passes the frame buffer buffer pointer to gst-dsp > and the tidspbridge driver, which maps them to the DSP IOMMU address space. Frame buffer sounds like kernel memory not user's, this because tidspbridge map functions do get_user_pages on the memory passed to it, unless you set VM_IO (I believe, so tidspbridge just does a get_page), however the issue might be related with the page count in the end. > When starting the JPEG encoder a bunch of identical messages are printed to > the kernel log: > > [14643.065490] BUG: Bad page state in process source:src pfn:89e39 > [14643.072143] page:c0b11720 count:0 mapcount:0 mapping: (null) index:0x0 > [14643.079437] page flags: 0x410(dirty|reserved) > [14643.084197] [] (unwind_backtrace+0x0/0xe0) from [] > (bad_page+0xc4/0xec) > [14643.093505] [] (bad_page+0xc4/0xec) from [] > (free_pages_prepare+0x70/0xe8) > [14643.102935] [] (free_pages_prepare+0x70/0xe8) from [] > (free_hot_cold_page+0x20/0x1ac) > [14643.113525] [] (free_hot_cold_page+0x20/0x1ac) from [] > (bridge_brd_mem_un_map+0x12c/0x3d4 [bridgedriver]) > [14643.126007] [] (bridge_brd_mem_un_map+0x12c/0x3d4 [bridgedriver]) > from [] (proc_un_map+0x6c/0xa0 [bridgedriver]) > [14643.139404] [] (proc_un_map+0x6c/0xa0 [bridgedriver]) from > [] (api_call_dev_ioctl+0xe8/0x10c [bridgedriver]) > [14643.152160] [] (api_call_dev_ioctl+0xe8/0x10c [bridgedriver]) > from [] (bridge_ioctl+0x12c/0x15c [bridgedriver]) > [14643.165191] [] (bridge_ioctl+0x12c/0x15c [bridgedriver]) from > [] (do_vfs_ioctl+0x4e8/0x55c) > [14643.176177] [] (do_vfs_ioctl+0x4e8/0x55c) from [] > (sys_ioctl+0x34/0x54) > [14643.185333] [] (sys_ioctl+0x34/0x54) from [] > (ret_fast_syscall+0x0/0x3c) > > The backtrace is printed because the page has the PG_reserved flag set, which > is expected (as far as I know) for the frame buffer memory. Yes, PG_reserved is set, and given that tidspbridge is unmapping, it is also telling the kernel that the pages are not going to be used anymore, however in the beginning the memory was allocated by either frame buffer or display driver. I believe the issue is because the page count reaches to 0 but PG_reserved is set as you point out. > Is this use case unsupported ? Not entirely supported, as long as the memory used by other drivers increases the page count of each page there shouldn't be any issue, since bridge doesn't know which pages have more than X number of users. 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 v4 1/4] OMAP3: hwmod data: add mmu data for iva and isp
Hi Laurent On Sun, Dec 25, 2011 at 3:08 PM, Laurent Pinchart wrote: >> > I'm not sure how this clock stuff works, but I'm guessing the device >> > is supposed to go to sleep at some points in time, and with your patch >> > "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module >> > is loaded, unless I'm missing something. >> >> The device should be able to be put to sleep at anytime when it is NOT >> being used. AFAIK there is no mechanism for the main processor (the >> one running the kernel) to know when the other iommus are not being >> used, given that they are in independent processors/subsystems, at >> least for the ones in the DSP or M3 processors. Once the user releases >> its iommu resource it means it is no longer using it, at that point >> the device can be put to sleep. > > How should the OMAP3 ISP driver proceed to make sure that its IOMMU is clocked > off when it doesn't need it ? If there is an specific scenario where the iommu should be disabled, iommu_detach_device can be called to release the iommu resource. On suspend/resume scenarios runtime pm callbacks should still be able to put the device in idle. 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 v4 4/4] OMAP3/4: iommu: adapt to runtime pm
On Tue, Dec 27, 2011 at 3:41 AM, Felipe Contreras wrote: > On Sun, Dec 25, 2011 at 2:03 AM, Ramirez Luna, Omar > wrote: >> On Fri, Dec 23, 2011 at 11:04 AM, Felipe Contreras >> wrote: >>> Which omap-iommu? The platform driver, or the device stuff? The device >>> stuff is always built-in, but not the platform driver >>> (drivers/iommu/omap-iommu.c), that can be a module. >> >> Both, I can't recall exactly when it changed (prior to being moved to >> drivers/iommu it could be built as a module), but now >> CONFIG_OMAP_IOMMU is boolean type. > > I see. Still, it looks like the proper way to use the API is to call > _enable() on the platform driver. I'm sorry, I don't seem to follow... you want _enable in platform driver, and you call drivers/iommu/omap-iommu.c the platform driver... And that is precisely where pm_runtime_enable() is, no? 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 v4 4/4] OMAP3/4: iommu: adapt to runtime pm
On Fri, Dec 23, 2011 at 11:04 AM, Felipe Contreras wrote: > Which omap-iommu? The platform driver, or the device stuff? The device > stuff is always built-in, but not the platform driver > (drivers/iommu/omap-iommu.c), that can be a module. Both, I can't recall exactly when it changed (prior to being moved to drivers/iommu it could be built as a module), but now CONFIG_OMAP_IOMMU is boolean type. 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 v4 4/4] OMAP3/4: iommu: adapt to runtime pm
On Mon, Dec 19, 2011 at 10:27 AM, Felipe Contreras wrote: > On Fri, Dec 16, 2011 at 5:18 AM, Ramirez Luna, Omar > wrote: >> On Thu, Dec 15, 2011 at 6:53 PM, Felipe Contreras >> wrote: >>> On Thu, Dec 15, 2011 at 6:18 AM, Omar Ramirez Luna >>> wrote: >>>> Use runtime PM functionality interfaced with hwmod enable/idle >>>> functions, to replace direct clock operations, reset and sysconfig >>>> handling. >>>> >>>> Removed clk handling during interrupt, given that in order to receive one, >>>> the device should be powered on in advance. Now doing pm_runtime_get/put >>>> on iommu_enable/disable so it doesn't rely on others to keep the clocks on. >>>> >>>> Signed-off-by: Omar Ramirez Luna >>>> --- >>>> arch/arm/mach-omap2/iommu2.c | 17 --- >>>> arch/arm/mach-omap2/omap-iommu.c | 1 - >>>> arch/arm/plat-omap/include/plat/iommu.h | 2 - >>>> arch/arm/plat-omap/include/plat/iommu2.h | 2 - >>>> drivers/iommu/omap-iommu.c | 44 >>>> - >>>> 5 files changed, 18 insertions(+), 48 deletions(-) >>> >>> Shouldn't pm_runtime_enable() be called in omap_iommu_init(), and >>> omap_iommu_probe() call pm_runtime_get_sync()/put() on the sections >>> where the device should be active? >> >> omap_iommu_init is called on module init however omap_iommu_probe is >> called by driver instance (isp, iva), on probe pm_runtime_enable >> activates runtime for both isp and iva devices, one at a time. > > Yes, but omap_iommu_init() will *always* be called at boot time and > will register the data for all the devices. There are 2 omap_iommu_init :/ Thought you were talking about the one in drivers/iommu/omap-iommu.c. > If the 'iommu' module is > never loaded, then the devices will remain active the whole time. oma-iommu is meant to be built-in as part of the kernel, there is no option for module anymore. 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 v4 1/4] OMAP3: hwmod data: add mmu data for iva and isp
On Mon, Dec 19, 2011 at 10:11 AM, Felipe Contreras
wrote:
> On Fri, Dec 16, 2011 at 4:01 AM, Ramirez Luna, Omar
> wrote:
>> On Thu, Dec 15, 2011 at 6:39 PM, Felipe Contreras
>> wrote:
>>> Are you sure you are not missing something like:
>>>
>>> .clk = "cam_ick",
>>
>> I believe in this case cam_ick is used as the main clock as it
>> supplies both functional and interface.
>
> Are you sure?
As sure as 4.7.4.1.7 CAM Power Domain ;), if someone else could
clarify would be great to avoid the "are you sure" discussion.
>>>> +/* isp mmu slave ports */
>>>> +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = {
>>>> + &omap3xxx_l4_core__isp_mmu,
>>>> +};
>>>> +
>>>> +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = {
>>>> + .name = "isp_mmu",
>>>> + .class = &omap3xxx_mmu_hwmod_class,
>>>> + .mpu_irqs = omap3xxx_isp_mmu_irqs,
>>>> + .main_clk = "cam_ick",
>>>
>>> It's not "cam_fck"?
>>
>> AFAIK cam_fck doesn't exist in the code, and CAM_L3_ICK is used as
>> both ick/fck according to TRM.
>
> Then maybe the code is wrong.
>
> Look at the OMAP34xx documentation, it says that:
>
> CAM_L3_ICLK -> CAM_FCLK
> CAM_L4_ICLK -> CAM_ICLK
> CAM_MCLK -> CAM_MCLK
> CSI2_96M_FCLK -> CSI2_96M_FCLK
>
> CAM_FCLK
> Functional clock (L3 interconnect clock domain)
> Functional clock domain.
>
> CAM_ICLK
> Interface clock (L4 interconnect clock domain)
> Interface clock domain.
"The camera subsystem interface is clocked with the L3 and L4 clocks
(CAM_L3_ICLK and CAM_L4_ICLK, respectively). CAM_L3_ICLK is also used
as the main functional clock. The functional clock (CAM_MCLK) is
provided by DPLL4 to supply the external sensor."
Either CAM subsystem section or PRCM - CAM PWRDM is ambiguous or wrong.
...
>
> Looks like the driver is manually calling clk_get() and clk_put() for
> the "i3_ick", where I guess the clock framework is supposed to do
> that... It's almost as if somebody forgot a dependency somewhere.
Would be good to clarify the intentions to keep the code as it is.
>>>> + .dev_attr = &isp_mmu_dev_attr,
>>>> + .slaves = omap3xxx_isp_mmu_slaves,
>>>> + .slaves_cnt = ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
>>>> + .flags = HWMOD_NO_IDLEST,
>>>> +};
>>>
>>> Most of the stuff I see the hwmods is .main_lock = "foo_fck", slave
>>> .clk = "foo_ick". Maybe that explains the irq issues you get.
>>
>> I see irq issues with iva hwmod because tidspbridge doesn't use iommu
>> API yet, so if you enable both the mmu hwmod and tidspbridge own mmu
>> implementation there will be some conflicts.
>>
>> I didn't see isp issues though, but I didn't went more than
>> booting/enabling with isp mmu.
>
> This is what you said:
> Removed clk handling during interrupt, given that in order to receive one,
> the device should be powered on in advance.
Yes, you should receive an interrupt if the clock is enabled and the
iommu is being used, hence the part inside in the ISR to enable the
clocks was removed.
> I'm not sure how this clock stuff works, but I'm guessing the device
> is supposed to go to sleep at some points in time, and with your patch
> "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module
> is loaded, unless I'm missing something.
The device should be able to be put to sleep at anytime when it is NOT
being used. AFAIK there is no mechanism for the main processor (the
one running the kernel) to know when the other iommus are not being
used, given that they are in independent processors/subsystems, at
least for the ones in the DSP or M3 processors. Once the user releases
its iommu resource it means it is no longer using it, at that point
the device can be put to sleep.
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 v4 4/4] OMAP3/4: iommu: adapt to runtime pm
On Thu, Dec 15, 2011 at 6:53 PM, Felipe Contreras wrote: > On Thu, Dec 15, 2011 at 6:18 AM, Omar Ramirez Luna > wrote: >> Use runtime PM functionality interfaced with hwmod enable/idle >> functions, to replace direct clock operations, reset and sysconfig >> handling. >> >> Removed clk handling during interrupt, given that in order to receive one, >> the device should be powered on in advance. Now doing pm_runtime_get/put >> on iommu_enable/disable so it doesn't rely on others to keep the clocks on. >> >> Signed-off-by: Omar Ramirez Luna >> --- >> arch/arm/mach-omap2/iommu2.c | 17 --- >> arch/arm/mach-omap2/omap-iommu.c | 1 - >> arch/arm/plat-omap/include/plat/iommu.h | 2 - >> arch/arm/plat-omap/include/plat/iommu2.h | 2 - >> drivers/iommu/omap-iommu.c | 44 >> - >> 5 files changed, 18 insertions(+), 48 deletions(-) > > Shouldn't pm_runtime_enable() be called in omap_iommu_init(), and > omap_iommu_probe() call pm_runtime_get_sync()/put() on the sections > where the device should be active? omap_iommu_init is called on module init however omap_iommu_probe is called by driver instance (isp, iva), on probe pm_runtime_enable activates runtime for both isp and iva devices, one at a time. 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 v4 4/4] OMAP3/4: iommu: adapt to runtime pm
On Thu, Dec 15, 2011 at 6:33 PM, Felipe Contreras wrote: > On Thu, Dec 15, 2011 at 6:18 AM, Omar Ramirez Luna > wrote: >> @@ -123,11 +123,10 @@ static int iommu_enable(struct omap_iommu *obj) >> if (!arch_iommu) >> return -ENODEV; >> >> - clk_enable(obj->clk); >> + pm_runtime_get_sync(obj->dev); >> >> err = arch_iommu->enable(obj); >> >> - clk_disable(obj->clk); >> return err; >> } > > Hmm, this is called on omap_iommu_attach, and iommu_disable is not > called until omap_iommu_detach, so this means the device will never > sleep. Why don't you call pm_runtime_put() instead of clk_disable()? Here once you call pm_runtime_put, the hwmod should turn off iva2_ck leaving device unpowered, not usable if it is part of an independent module and that module is still awake, but since the module clock feeds both the dsp and the mmu this doesn't occur. However might be possible if there is a specific clock to feed the mmu and another for the mmu user. In theory iommu should be independent on handling its clock resources, take tidspbridge, it powers iva2_clk which indirectly powers iva_mmu, so as long as the dsp is powered and using the mmu (omap_iommu_attach) the iommu should be ON, once you omap_iommu_detach means that the device is no longer using the iommu and can be put to sleep. This could be helpful if your main processor can go to sleep independently of other processors, say you attach the iommu and leave the dsp doing calculations and memory accesses through mmu while the main processor decides to enter sleep. > All the rest of the calls to pm_runtime_get/put after this are > basically no-ops, because the count will never go below 1. I didn't try but without calling iommu_attach the other functions that use pm_runtime_get/put can still be called as they are exported symbols. I believe omap-iommu-debug.c does something like this to dump stuff. This should be cleaned up/audited, IMHO, but it doesn't seem to belong to this conversion series. > You mention some irq issues, but could it be due to some bad clocks in > the hwmod data? There are no "issues" with irqs, just a weird case, see below... ... >> @@ -780,9 +777,7 @@ static irqreturn_t iommu_fault_handler(int irq, void >> *data) >> if (!obj->refcount) >> return IRQ_NONE; >> >> - clk_enable(obj->clk); >> errs = iommu_report_fault(obj, &da); >> - clk_disable(obj->clk); > > Why? In order to get an irq from the mmu, the mmu should be functional and have a clock, but iommu_enable used to enable and disable the clock. In the end you are able to get an irq because someone else (tidspbridge) has the mmu indirectly powered (since they share the same clock), I felt this shouldn't be and the iommu should handle its own clocks even if they are shared with other modules. Hence iommu_enable does pm_runtime_get for the life time of the user of the mmu. ... >> @@ -996,7 +987,8 @@ static int __devexit omap_iommu_remove(struct >> platform_device *pdev) >> release_mem_region(res->start, resource_size(res)); >> iounmap(obj->regbase); >> >> - clk_put(obj->clk); >> + pm_runtime_disable(obj->dev); > > I'm not sure if this is needed; you want to resume the driver? AFAICS > kfree will take care of that _without_ resuming. No, the driver should resume only if there are outstanding wake up requests, right? I don't seem to get this question. Thanks, 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 v4 1/4] OMAP3: hwmod data: add mmu data for iva and isp
On Thu, Dec 15, 2011 at 6:39 PM, Felipe Contreras
wrote:
> On Thu, Dec 15, 2011 at 6:18 AM, Omar Ramirez Luna
> wrote:
>> +/* l4_core -> isp mmu */
>> +static struct omap_hwmod_ocp_if omap3xxx_l4_core__isp_mmu = {
>> + .master = &omap3xxx_l4_core_hwmod,
>> + .slave = &omap3xxx_isp_mmu_hwmod,
>> + .addr = omap3xxx_isp_mmu_addrs,
>> + .user = OCP_USER_MPU | OCP_USER_SDMA,
>> +};
>
> Are you sure you are not missing something like:
>
> .clk = "cam_ick",
I believe in this case cam_ick is used as the main clock as it
supplies both functional and interface.
>
>> +/* isp mmu slave ports */
>> +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = {
>> + &omap3xxx_l4_core__isp_mmu,
>> +};
>> +
>> +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = {
>> + .name = "isp_mmu",
>> + .class = &omap3xxx_mmu_hwmod_class,
>> + .mpu_irqs = omap3xxx_isp_mmu_irqs,
>> + .main_clk = "cam_ick",
>
> It's not "cam_fck"?
AFAIK cam_fck doesn't exist in the code, and CAM_L3_ICK is used as
both ick/fck according to TRM.
>
>> + .dev_attr = &isp_mmu_dev_attr,
>> + .slaves = omap3xxx_isp_mmu_slaves,
>> + .slaves_cnt = ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
>> + .flags = HWMOD_NO_IDLEST,
>> +};
>
> Most of the stuff I see the hwmods is .main_lock = "foo_fck", slave
> .clk = "foo_ick". Maybe that explains the irq issues you get.
I see irq issues with iva hwmod because tidspbridge doesn't use iommu
API yet, so if you enable both the mmu hwmod and tidspbridge own mmu
implementation there will be some conflicts.
I didn't see isp issues though, but I didn't went more than
booting/enabling with isp mmu.
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] ARM: OMAP: mcbsp: Fix possible memory corruption
On Mon, Dec 12, 2011 at 12:33 PM, Tony Lindgren wrote: > Applying into fixes. FWIW, I was bisecting this issue, the patch fixes the following slab corruptions on boot log: ... hw-breakpoint: found 5 (+1 reserved) breakpoint and 1 watchpoint registers. hw-breakpoint: maximum watchpoint size is 4 bytes. Slab corruption: size-32 start=eea66740, len=32 010: d8 9d 02 c0 04 9d 02 c0 6b 6b 6b 6b 6b 6b 6b a5 kkk. Prev obj: start=eea66720, len=32 000: 6f 6d 61 70 32 5f 6d 63 73 70 69 2e 31 00 5a 5a omap2_mcspi.1.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Next obj: start=eea66760, len=32 000: 70 6f 77 65 72 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a power.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Slab corruption: size-32 start=eea66740, len=32 010: d8 9d 02 c0 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkk. Prev obj: start=eea66720, len=32 000: 6f 6d 61 70 32 5f 6d 63 73 70 69 2e 31 00 5a 5a omap2_mcspi.1.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Next obj: start=eea66760, len=32 000: 70 6f 77 65 72 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a power.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Slab corruption: size-32 start=eea66740, len=32 010: d8 9d 02 c0 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkk. Prev obj: start=eea66720, len=32 000: 6f 6d 61 70 32 5f 6d 63 73 70 69 2e 31 00 5a 5a omap2_mcspi.1.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Next obj: start=eea66760, len=32 000: 70 6f 77 65 72 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a power.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Slab corruption: size-32 start=eea66740, len=32 010: d8 9d 02 c0 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkk. Prev obj: start=eea66720, len=32 000: 6f 6d 61 70 32 5f 6d 63 73 70 69 2e 31 00 5a 5a omap2_mcspi.1.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. Next obj: start=eea66760, len=32 000: 70 6f 77 65 72 00 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a power.ZZ 010: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a a5 ZZZ. OMAP DMA hardware revision 0.0 ... 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 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context
On Mon, Dec 12, 2011 at 5:08 PM, Tony Lindgren wrote:
...
>> @@ -134,22 +134,13 @@ static void omap_dm_timer_reset(struct
>> omap_dm_timer *timer)
>> int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>> {
>> ...
>> - ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
>> ...
>>
>> All clocks requested are set to 32 KHz first, even with the current
>> approach, there exists another API to set a new source.
>>
>> To be honest I don't know why the clocks are set to 32 KHz first,
>> maybe the default call path for users should be:
>
> You need a functional clock for the timer registers to work
> I believe.
Sounds logic :)
>> omap_dm_timer_request
>
> Yes this would make sense. The omap_timer_list should be protected
> by a mutex. There should not be a need for spinlock there as
> omap_dm_timer_request should be only called during init. If that's
> not the case, the the driver using it is broken.
Ok, I made this patch thinking that 'request' could be called from any
context, but if that is not the case mutex should be fine.
>> omap_dm_timer_set_source (new explicit call)
>> omap_dm_timer_start
>
> Once the timer has been requested, there should not be a need
> for locking as there should be only one timer user using the
> timer specific registers.
>
>> And remove setting the source to 32 KHz by default in omap_dm_timer_request.
>
> That you may need to be able to do anything with the timer :)
Well the intention was for the user to call it explicitly so it didn't
look as a hard coded setting, but I can keep it.
IIUC, mutex should be held instead of spin lock, I can do the change
instead of this patch and see how it goes.
Thanks,
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] ARM: remove header files included more than once
Hi Haojian, On Sun, Dec 11, 2011 at 7:10 PM, Haojian Zhuang wrote: ... > Hi Omar, > > Thanks for your patch. Could you help to split your patches into small > pieces? Since different people are handling for different files. For > example, I can help to handle files in arch-pxa. But I can't handle > other files. No problem, I'll split the patch to be included by independent trees. Thanks, 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] staging: tidspbridge: request dmtimer clocks on init
On Fri, Dec 9, 2011 at 4:38 PM, Greg KH wrote: > How about for 3.2.1? I'll try to get this into 3.2, but I can't > guarantee anything so late in the release cycle, sorry. Sounds fair, fingers crossed for 3.2 ;) Thanks, 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 1/2] ARM: OMAP: dmtimer: fix sleeping function called from invalid context
On Fri, Dec 9, 2011 at 3:34 PM, Tony Lindgren wrote:
>> + ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
...
>> EXPORT_SYMBOL_GPL(omap_dm_timer_request);
>
> This does not seem right.. It seems that you're hardcoding the source
> clock to 32 KiHz clock while other sources are available too?
Agree, but... (below)
>> + ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
...
> And here too?
Agree but that is the default behavior set by dm timer framework:
@@ -134,22 +134,13 @@ static void omap_dm_timer_reset(struct
omap_dm_timer *timer)
int omap_dm_timer_prepare(struct omap_dm_timer *timer)
{
...
- ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
...
All clocks requested are set to 32 KHz first, even with the current
approach, there exists another API to set a new source.
To be honest I don't know why the clocks are set to 32 KHz first,
maybe the default call path for users should be:
omap_dm_timer_request
omap_dm_timer_set_source (new explicit call)
omap_dm_timer_start
And remove setting the source to 32 KHz by default in omap_dm_timer_request.
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] staging: tidspbridge: request dmtimer clocks on init
On Thu, Dec 8, 2011 at 6:07 PM, Greg KH wrote: >> > Definitely 3.2-fix material IMO :) >> >> I believe it was pushed to staging-next... I'm rooting so it can be >> pushed to staging-linus :) > > As it wasn't originally stated that this was a problem that needed to be > fixed for 3.2, can it wait for 3.3-rc1? Especially given that there is > still discussion about this? No discussion from me either. The original intention was to include this patch for 3.2 because of the recent changes in dm timer framework, I apologize if "This was first seen on 3.2-rc1." is too ambiguous. I hope this can make it for 3.2 though. Thanks, 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] staging: tidspbridge: request dmtimer clocks on init
On Thu, Dec 8, 2011 at 2:11 PM, Felipe Contreras wrote: > On Sat, Nov 19, 2011 at 12:18 AM, Omar Ramirez Luna > wrote: >> Given that dm timer framework doesn't support request of clocks >> by soft | hard irqs because some recent changes, tidspbridge needs >> to request its clocks on init and enable/disable them on demand. >> >> This was first seen on 3.2-rc1. >> >> Signed-off-by: Omar Ramirez Luna > > Tested-by: Felipe Contreras > Reviewed-by: Felipe Contreras Thanks! > Something the commit message didn't mention is that this fixes a nasty > continuous loop that happens as soon as you load the module. Indeed. > Definitely 3.2-fix material IMO :) I believe it was pushed to staging-next... I'm rooting so it can be pushed to staging-linus :) 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] staging: tidspbridge: request dmtimer clocks on init
On Thu, Dec 8, 2011 at 1:50 PM, Felipe Contreras wrote: > On Thu, Dec 8, 2011 at 9:10 PM, Ramirez Luna, Omar > wrote: >> On Wed, Dec 7, 2011 at 4:11 PM, Felipe Contreras >> wrote: >>> On Sat, Nov 19, 2011 at 12:18 AM, Omar Ramirez Luna >>> wrote: >>>> Given that dm timer framework doesn't support request of clocks >>>> by soft | hard irqs because some recent changes, tidspbridge needs >>>> to request its clocks on init and enable/disable them on demand. >>>> >>>> This was first seen on 3.2-rc1. >>>> >>>> Signed-off-by: Omar Ramirez Luna >>> >>> This looks similar similar to this one: >>> http://article.gmane.org/gmane.linux.ports.arm.omap/61816 >>> >>> Are you sure this is what we want instead of that one? >> >> I believe your patch only takes care of gpt5 and gpt6, but not of the >> other 2 that could be requested by the dsp, let's say gpt8 for avsync. > > I'm not really sure about that... Judging from the code, the only > timers needed used to be initialized on bridge_brd_start based on the > symbols of the baseimage. If you say that's not enough, then I guess > that's fine (not really surprising that the old code missed that). I meant the dsp by itself can generate that request. That is why it is not explicitly seen in the code. >> - Handles the other gpt that can be requested by the dsp. > > And I guess which clocks are going to be used is not discoverable somehow... It is basically the pool of clocks of the dsp is formed by mcbsp, gpt, wdt, ssi. The dsp can request any gpt from 5 to 8 according to the binary you are running on the dsp side. That's the reason it is somewhat hidden what clock the dsp needs. >> - Uses start/stop according to future plans of making enable/disable static. > > Yeah, but that's easy to change s/enable/start/ s/disable/stop/ Agree >> - Might be temporal if *dm_timer_request* functions are fixed and >> there is an overall feeling that we can revert to use >> request+start/stop+free on request. > > At which point we would end up with something similar to my patch :) Yes, plus handling the other clocks that can be available to the dsp. > Anyway, I guess there's not much problem in requesting extra clocks > that we would not use. AV playback does use gpt8, but for systems that doesn't use gpt8 it is best to use the old approach of just request/free prior to dm timer changes. 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] staging: tidspbridge: request dmtimer clocks on init
On Wed, Dec 7, 2011 at 4:11 PM, Felipe Contreras wrote: > On Sat, Nov 19, 2011 at 12:18 AM, Omar Ramirez Luna > wrote: >> Given that dm timer framework doesn't support request of clocks >> by soft | hard irqs because some recent changes, tidspbridge needs >> to request its clocks on init and enable/disable them on demand. >> >> This was first seen on 3.2-rc1. >> >> Signed-off-by: Omar Ramirez Luna > > This looks similar similar to this one: > http://article.gmane.org/gmane.linux.ports.arm.omap/61816 > > Are you sure this is what we want instead of that one? I believe your patch only takes care of gpt5 and gpt6, but not of the other 2 that could be requested by the dsp, let's say gpt8 for avsync. As for this patch, dm timer framework should support request on soft or hard irq[1], but it can't because the underlying functions to use clk_get->clk_get_sys which holds a mutex. I believe your error was seen because of patches not in mainline, but the concept between the patches is similar indeed, however mine: - Handles the other gpt that can be requested by the dsp. - Uses start/stop according to future plans of making enable/disable static. - Might be temporal if *dm_timer_request* functions are fixed and there is an overall feeling that we can revert to use request+start/stop+free on request. Regards, Omar, --- [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg58665.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] ARM: OMAP: dmtimer: fix missing content/correction in low-power mode support
Hi,
On Tue, Nov 8, 2011 at 12:00 AM, Tarun Kanti DebBarma
wrote:
> Since omap_dm_timer_write_reg/__omap_dm_timer_write is now modified
> to use timer->func_base OCP_CFG should not use this wrapper anymore.
> Instead use __raw_writel() directly and use timer->io_base instead
> to write to OCP_CFG.
>
> The timer->sys_stat is valid only if timer->revision is 1. In the
> context restore function make this correction.
>
> Save the contexts and loss count when timer is stopped.
> Also, disable the clock. Else, clock usecount would become imbalanced.
>
> Signed-off-by: Tarun Kanti DebBarma
...
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index af3b92b..f042c82 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
...
> @@ -357,6 +357,21 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
>
> __omap_dm_timer_stop(timer, timer->posted, rate);
>
> + if (timer->loses_context) {
> + if (timer->get_context_loss_count)
Maybe: if (timer->loses_context && timer->get_context_loss_count)
> + timer->ctx_loss_count =
> + timer->get_context_loss_count(&timer->pdev->dev);
could get rid of this weird one-line formatting
> + }
> +
> + /*
> + * Since the register values are computed and written within
> + * __omap_dm_timer_stop, we need to use read to retrieve the
> + * context.
> + */
> + timer->context.tclr =
> + omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
> + timer->context.tisr = __raw_readl(timer->irq_stat);
> + omap_dm_timer_disable(timer);
FWIW, functionally it looks good to me, besides it fixes the broken
dmtimer start/stop sequence from 3.2-rc1. Tested with tidspbridge on
omap3.
If needed:
Tested-by: Omar Ramirez Luna
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] staging: tidspbridge: request dmtimer clocks on init
Hi Greg, On Fri, Nov 18, 2011 at 3:54 PM, Omar Ramirez Luna wrote: > diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c > b/drivers/staging/tidspbridge/core/dsp-clock.c > index 3d1279c..1ba10ae 100644 > --- a/drivers/staging/tidspbridge/core/dsp-clock.c > +++ b/drivers/staging/tidspbridge/core/dsp-clock.c > @@ -54,6 +54,7 @@ > > /* Bridge GPT id (1 - 4), DM Timer id (5 - 8) */ > #define DMT_ID(id) ((id) + 4) > +#define DM_TIMER_CLOCKS 5 :/ typo, please ignore this patch there are only 4 clocks for the dsp. I'll send v2 instead. Thanks, 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 v3 2/4] OMAP4: hwmod data: add mmu hwmod for ipu and dsp
Hi, On Fri, Nov 4, 2011 at 6:23 PM, Kevin Hilman wrote: >> + .flags = HWMOD_INIT_NO_RESET, > > Why is this needed? ... >> + .flags = HWMOD_INIT_NO_RESET, > > And this? I have this because the hwmod complains about a failure in hard reset, even though the reset deassert does complete after the clock is enabled. Later on, hwmod will warn again because of a wrong state when enabling, I believe because of the failure on _setup but didn't dig into it yet. 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
Hi, On Fri, Nov 4, 2011 at 6:27 PM, Kevin Hilman wrote: >> @@ -821,9 +820,7 @@ static irqreturn_t iommu_fault_handler(int irq, void >> *data) >> if (!obj->refcount) >> return IRQ_NONE; >> >> - clk_enable(obj->clk); >> errs = iommu_report_fault(obj, &da); >> - clk_disable(obj->clk); >> if (errs == 0) >> return IRQ_HANDLED; > > I'm not terribly familiar with this IOMMU code, but this one looks > suspiciou because you're removing the clock calls but not replacing them > with runtime PM get/put calls. > > I just want to make sure that's intentional. If so, you might want to > add a comment about that to the changelog. Yes it is intentional, reason is that in order to get an interrupt, the device should be powered on in advance, right now it is working because the modules share a common clock so the users of the omap-iommu indirectly give power to it. However I made another change to do pm_runtime_get/put on attach/detach so it doesn't rely on others to keep the clocks on. I'll add the comment. Thanks, 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
On Wed, Nov 2, 2011 at 10:21 AM, Russell King - ARM Linux wrote: > On Tue, Nov 01, 2011 at 05:15:52PM -0500, Omar Ramirez Luna wrote: >> Use runtime PM functionality interfaced with hwmod enable/idle >> functions, to replace direct clock operations, reset and sysconfig >> handling. >> >> Tidspbridge uses a macro removed with this patch, for now the value >> is hardcoded to avoid breaking compilation. > > You probably want to include people involved with power management on > this, so maybe the linux-pm mailing list, and those involved with > runtime-pm stuff (I think Rafael qualifies as the maintainer for this > stuff, even if he's not listed in MAINTAINERS.) Will do, I'll submit it again if no other comments are received. 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 v3 4/4] OMAP3/4: iommu: adapt to runtime pm
Hi MyungJoo, On Wed, Nov 2, 2011 at 5:16 AM, MyungJoo Ham wrote: > On Wed, Nov 2, 2011 at 7:15 AM, Omar Ramirez Luna wrote: >> Use runtime PM functionality interfaced with hwmod enable/idle >> functions, to replace direct clock operations, reset and sysconfig >> handling. >> >> Tidspbridge uses a macro removed with this patch, for now the value >> is hardcoded to avoid breaking compilation. >> >> Signed-off-by: Omar Ramirez Luna >> --- >> arch/arm/mach-omap2/iommu2.c | 17 >> arch/arm/mach-omap2/omap-iommu.c | 1 - >> arch/arm/plat-omap/include/plat/iommu.h | 2 - >> arch/arm/plat-omap/include/plat/iommu2.h | 2 - >> drivers/iommu/omap-iommu.c | 46 >> - >> drivers/staging/tidspbridge/core/tiomap3430_pwr.c | 2 +- >> 6 files changed, 19 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c >> index bbbf747..3c55be0 100644 >> --- a/drivers/iommu/omap-iommu.c >> +++ b/drivers/iommu/omap-iommu.c >> @@ -123,11 +123,11 @@ static int iommu_enable(struct omap_iommu *obj) >> if (!arch_iommu) >> return -ENODEV; >> >> - clk_enable(obj->clk); >> + pm_runtime_enable(obj->dev); >> + pm_runtime_get_sync(obj->dev); >> >> err = arch_iommu->enable(obj); >> >> - clk_disable(obj->clk); >> return err; >> } >> >> @@ -136,11 +136,10 @@ static void iommu_disable(struct omap_iommu *obj) >> if (!obj) >> return; >> >> - clk_enable(obj->clk); >> - >> arch_iommu->disable(obj); >> >> - clk_disable(obj->clk); >> + pm_runtime_put_sync(obj->dev); >> + pm_runtime_disable(obj->dev); >> } > I'm just curious here... Is there any reason to do > pm_runtime_enable/disable at iommu_enable/iommu_disable which are > called by iommu_attach/detach? > I thought that normally, ideal locations of pm_runtime_enable/disable > for such devices are in probe/remove() because it assures that the > device is suspended after the probe. > It seems that the device might be kept on after probe and before the > first iommu_attach if it is default-on. The default state of these MMUs is on reset and needs to be deasserted to be used, but you're right, it makes more sense to move pm_runtime_enable/disable calls to probe/remove. I'll wait a bit, do this change and resubmit. Thanks for the comment, 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 v3 0/4] OMAP: iommu: hwmod support and runtime PM
+ Joerg, iommu-list On Tue, Nov 1, 2011 at 5:15 PM, Omar Ramirez Luna wrote: > Introduced hwmod support for OMAP3 (iva, isp) and OMAP4 (ipu, dsp), > along with the corresponding runtime PM routines to deassert reset > lines, enable/disable clocks and configure sysc registers. > > v3: > - Rebased to 3.1-rc10 lo rebuilt, added structure terminators, and > removed .omap_chip field. > > v2: > - Added oh reset info to assert/deassert mmu reset lines. > - Addressed previous comments on v1 > http://www.spinics.net/lists/arm-kernel/msg103271.html > > Due to compatibility an ifdef needs to be propagated (previously on > iommu resource info) to hwmod data in OMAP3, so users of iommu and > tidspbridge can avoid issues of two modules managing mmu data/irqs/resets; > this until tidspbridge can be safely migrated to iommu framework. > > Omar Ramirez Luna (4): > OMAP3: hwmod data: add mmu data for iva and isp > OMAP4: hwmod data: add mmu hwmod for ipu and dsp > OMAP3/4: iommu: migrate to hwmod framework > OMAP3/4: iommu: adapt to runtime pm > > arch/arm/mach-omap2/iommu2.c | 36 - > arch/arm/mach-omap2/omap-iommu.c | 162 > - > arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 131 + > arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 154 ++-- > arch/arm/plat-omap/include/plat/iommu.h | 17 ++- > arch/arm/plat-omap/include/plat/iommu2.h | 2 - > drivers/iommu/omap-iommu.c | 49 +++ > drivers/media/video/omap3isp/isp.c | 2 +- > drivers/staging/tidspbridge/core/tiomap3430_pwr.c | 2 +- > 9 files changed, 339 insertions(+), 216 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
Re: tidspbridge issue with omap_dm_timer_free
Hi, On Sun, Aug 14, 2011 at 2:44 AM, Felipe Contreras wrote: > Yeah, but with the current approach it would be possible that > everything works fine, and then the DSP goes to hibernation, a module > is loaded that uses one dm timer, and then when the DSP wakes up, that > timer would fail, there isn't even a check for that. If I follow the > code correctly, when the DSP goes back to hibernation, there would be > a crash, as a NULL timer would be freed. I think that's too error > prone. Yes, this scenario could trigger such error condition. So, FWIW, I'm fine with just a check or your approach. If possible to apply the same mechanism for the other clocks, so in the enable/disable functions all the clocks are then enabled/disabled avoiding mixed behavior of enable/request, disable/free between gpt and mcbsp clocks. 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: tidspbridge issue with omap_dm_timer_free
On Wed, Aug 10, 2011 at 6:27 PM, Felipe Contreras wrote: > On Wed, Aug 10, 2011 at 9:42 PM, Felipe Contreras > wrote: >> I am trying to use a more recent version of the tidspbridge code in >> the Nokia N9, but I'm stuck with this warning that is caused by using >> the dm timer framework. > > Actually, the problem only happens on the 'dspbridge' branch, which > has some unmerged patches. These patches introduce some new mutexes > that trigger this. The only lock introduced is a spin_lock_bh which makes sense to warn if the underlying functions have a mutex, but I couldn't find the clk_set_parent mutex which is causing this on the omap files. > My proposed solution is to request the dm timers at initialization > time, and just enable/disable them on wake/hibernate, which is a bit > similar to what was happening before, and probably more efficient and > proper. When I made these changes I wanted to avoid keeping an array with the clocks requested and nobody else being able to use them (even if that meant a conflict between two drivers trying to use the same clock, but other warnings could help to point to such cases). Also I wanted to maintain uniformity, not just requesting some clock, freeing them and then some others to be held and freed only on module removal. That said, I have no problem on changing to your approach if needed. Regards, Omar > > I'm attaching the patch. > > -- > Felipe Contreras > -- 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: tidspbridge issue with omap_dm_timer_free
On Wed, Aug 10, 2011 at 1:42 PM, Felipe Contreras
wrote:
> Hi,
>
> I am trying to use a more recent version of the tidspbridge code in
> the Nokia N9, but I'm stuck with this warning that is caused by using
> the dm timer framework.
>
> [ 30.883636] BUG: sleeping function called from invalid context at
> kernel/mutex.c:287
> [ 30.885925] in_atomic(): 1, irqs_disabled(): 0, pid: 305, name: mboxd/0
> [ 30.892517] 3 locks held by mboxd/0/305:
> [ 30.896392] #0: (mboxd){+.+...}, at: []
> worker_thread+0x154/0x2bc
> [ 30.903533] #1: (&mq->work){+.+...}, at: []
> worker_thread+0x154/0x2bc
> [ 30.911041] #2: (pwr_lock){+.}, at: []
> handle_hibernation_from_dsp+0x1c/0x158 [bridgedriver]
> [ 30.920959] [] (unwind_backtrace+0x0/0xd4) from
> [] (mutex_lock_nested+0x30/0x32c)
> [ 30.930175] [] (mutex_lock_nested+0x30/0x32c) from
> [] (clk_set_parent+0x34/0xf8)
...
> From what I can see this could be triggered in upstream by enabling PM
> and debug mutex stuff right after loading the baseimage.
>
> Any ideas?
May I know which kernel version are you using? I can't seem to find
the mutex_lock inside clk_set_parent in arch/arm/plat-omap/clock.c.
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: tidspbridge: problems executing sample apps
Hi Javier, On Wed, Jun 15, 2011 at 10:19 AM, javier Martin wrote: > Hi, > > 1- If I try prebuilt binaries (both for DSP side and userspace) I get > an execution error: ... > DSPNode_GetMessage failed: 0xffc2 In this case, sample application is working, however the failures on get_message are unexpected, it seems this arose when the error codes were changed and the precompiled binaries where not updated, I'll fix this, thanks for the heads up. > 3- If I compile both DSP side and MPU side samples it also gives an > execution error: ... > root@beagleboard:/dspbridge# ./ping.out > DSP device detec[ 64.484649] procwrap_detach: deprecated dspbridge ioctl > ted !! > DSPNode_Create failed: 0xffe3 > ... > Do you know what combination of DSP bios, TI CGT and xdctools is most > suitable for using with tidspbridge in kernel 2.6.39? This is mentioned in the wiki: bios 5.33.04 and cgt tools 6.0.7 http://omapedia.org/wiki/DSPBridge_Project#Initial_environment_setup This version of the bios already has the xdc tools bundled. BTW I replied to your gforge query in case it gives any information: http://bit.ly/laeJFD I also applied your patch to fix copying the dsp binaries on the build system, thanks! 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 0/8] staging: tidspbridge: PM routines cleanup
On Wed, Mar 23, 2011 at 1:49 PM, Omar Ramirez Luna wrote: > Cleanup and fixes for tidspbridge's PM routines. > > The most significant ones are: > - Protection of critical sections when disabling clocks which > could cause issues during PM transitions. > - Fix of resume path from RET power state. > > Other patches are cleanups and preparations for the usage of > spinlock. > > Omar Ramirez Luna (8): > staging: tidspbridge: make wake_dsp to handle PM code > staging: tidspbridge: fix resume path from retention > staging: tidspbridge: remove msleep for dsp transition wait > staging: tidspbridge: send mbox PM command directly > staging: tidspbridge: send wake message even if dsp is running > staging: tidspbridge: remove redundant code from PM routines > staging: tidspbridge: remove redundant indentation in PM routines > staging: tidspbridge: protect critical sections on PM routines > > drivers/staging/tidspbridge/core/tiomap3430_pwr.c | 248 > ++--- > drivers/staging/tidspbridge/core/tiomap_io.c | 58 +- > 2 files changed, 168 insertions(+), 138 deletions(-) Pushed to dspbridge. 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] OMAP: iovmm: fix SW flags passed by user
Hi Hiroshi, On Fri, Mar 25, 2011 at 3:04 PM, Omar Ramirez Luna wrote: > Commit d038aee24dcd5a2a0d8547f5396f67ae9698ac8e > "omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag", > changes iovmm to receive flags specified by user, however > the upper 16 bits of the flags are wiped by iovmm itself. > > This fixes IOVMF_DA_FIXED flags from being lost, and lets the user > map its desired "device addresses". > > Signed-off-by: Omar Ramirez Luna > --- > > v2: > Include missing reference (commit and name) to patch in > description. If no comments, could you ack this patch? 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: HELP:dspbridge:how to configure GPT timer
Hi, On Sat, Apr 9, 2011 at 7:38 AM, onlyfever wrote: > Hi all! > currently dsp make use of gpt 5, 6, 7 or 8. > I'm using gpt 8 for something else. > Is it possible to configure dsp-bridge to use other gpt timer? > I want to change gpt 8 to gpt 9,or gpt 7. > Please give me some advice. GPT8 is used for avsync on the dsp side, unfortunately there is no way to change this without dsp sources and the latter are not publicly available :/ 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 0/5] OMAP: l3: fixes and cleanup
On Wed, Mar 30, 2011 at 1:45 AM, Santosh Shilimkar wrote: >> OMAP3: l3: fix for "irq 10: nobody cared" message >> OMAP3: l3: fix omap3_l3_probe error path >> OMAP3: l3: minor cleanup for error message, parenthesis and extra >> lines >> OMAP4: l3: fix omap4_l3_probe error path >> OMAP4: l3: minor cleanup for parenthesis and extra spaces >> > Thanks for the cleanup. I have reviewed the series and it looks > good to me. > I would suggest to fold similar changes like, > - PATCH 2/5 and PATCH 4/5 into one patch > - PATCH 3/5 and PATCH 5/5 into one patch > > With this update you can add my ack for this series Ok, will do, thanks for the comments. 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] OMAP3: l3: fix for "irq 10: nobody cared" message
Hi, On Mon, Mar 28, 2011 at 12:38 AM, Santosh Shilimkar wrote: >> If an error occurs in the L3 on any other initiator than MPU, >> the interrupt goes unhandled given that the 'base' register >> was calculated with the initialized err_base value (which >> coincidentally points to MPU) and not with the actual source >> of the error. >> >> Signed-off-by: Omar Ramirez Luna > > Patch looks good. Did you observe this with DSP initiator?? Yes, when loading a base image for the DSP, I got an: In-band Error Error seen by IVA_SS at address 0; after this fix. I was planning to remove the duplicated "Error" print too. > Acked-by: Santosh Shilimkar Thanks, I'll add it in my next version when I make the changes affecting only these lines. 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] OMAP3: l3: fix for "irq 10: nobody cared" message
On Sat, Mar 26, 2011 at 4:38 PM, Sergei Shtylyov wrote: > Hello. > > On 26-03-2011 3:29, Omar Ramirez Luna wrote: > >> If an error occurs in the L3 on any other initiator than MPU, >> the interrupt goes unhandled given that the 'base' register >> was calculated with the initialized err_base value (which >> coincidentally points to MPU) and not with the actual source >> of the error. > >> Signed-off-by: Omar Ramirez Luna >> --- >> arch/arm/mach-omap2/omap_l3_smx.c | 5 +++-- >> 1 files changed, 3 insertions(+), 2 deletions(-) > >> diff --git a/arch/arm/mach-omap2/omap_l3_smx.c >> b/arch/arm/mach-omap2/omap_l3_smx.c >> index 5f2da75..da917c2 100644 >> --- a/arch/arm/mach-omap2/omap_l3_smx.c >> +++ b/arch/arm/mach-omap2/omap_l3_smx.c >> @@ -196,11 +196,12 @@ static irqreturn_t omap3_l3_app_irq(int irq, void >> *_l3) >> /* No timeout error for debug sources */ >> } >> >> - base = ((l3->rt) + (*(omap3_l3_bases[int_type] + err_source))); >> - >> /* identify the error source */ >> for (err_source = 0; !(status& (1<< err_source)); err_source++) >> ; >> + >> + base = ((l3->rt) + (*(omap3_l3_bases[int_type] + err_source))); >> + > > What's the point of having () around rvalue? You could drop them, while at > it... You're right, will do. 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] OMAP3: l3: fix for "irq 10: nobody cared" message
On Sun, Mar 27, 2011 at 8:30 PM, Felipe Contreras wrote: > On Sat, Mar 26, 2011 at 2:29 AM, Omar Ramirez Luna > wrote: >> If an error occurs in the L3 on any other initiator than MPU, >> the interrupt goes unhandled given that the 'base' register >> was calculated with the initialized err_base value (which >> coincidentally points to MPU) and not with the actual source >> of the error. >> >> Signed-off-by: Omar Ramirez Luna >> --- >> arch/arm/mach-omap2/omap_l3_smx.c | 5 +++-- >> 1 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/omap_l3_smx.c >> b/arch/arm/mach-omap2/omap_l3_smx.c >> index 5f2da75..da917c2 100644 >> --- a/arch/arm/mach-omap2/omap_l3_smx.c >> +++ b/arch/arm/mach-omap2/omap_l3_smx.c >> @@ -196,11 +196,12 @@ static irqreturn_t omap3_l3_app_irq(int irq, void *_l3) >> /* No timeout error for debug sources */ >> } >> >> - base = ((l3->rt) + (*(omap3_l3_bases[int_type] + err_source))); >> - >> /* identify the error source */ >> for (err_source = 0; !(status & (1 << err_source)); err_source++) >> ; >> + >> + base = ((l3->rt) + (*(omap3_l3_bases[int_type] + err_source))); >> + >> error = omap3_l3_readll(base, L3_ERROR_LOG); > > One extra space too much. Between base and error assignments? Yep, I can remove it. 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 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
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. 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. 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 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
Re: [PATCH] OMAP3: l3: fix for "irq 10: nobody cared" message
On Fri, Mar 25, 2011 at 7:29 PM, Omar Ramirez Luna wrote: > If an error occurs in the L3 on any other initiator than MPU, > the interrupt goes unhandled given that the 'base' register > was calculated with the initialized err_base value (which s/err_base/err_source/ > coincidentally points to MPU) and not with the actual source > of the error. > > Signed-off-by: Omar Ramirez Luna 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] OMAP: iovmm: fix SW flags passed by user
On Fri, Mar 25, 2011 at 3:05 PM, Russell King - ARM Linux >> >> Commit d038aee24dcd changes iovmm to receive flags specified by user, >> > >> > Please also specify that commit's summary for the human readers. Bare ID >> > is only usable to gitweb... >> >> Here it is the patch, introducing the change: >> >> https://patchwork.kernel.org/patch/620871/ >> >> If accepted by Hiroshi, I can update the description making reference >> to this patch instead of commit. > > It is a requirement that all descriptions which refer to a commit shall > contain both the commit ID *and* the summary line for that commit. > Failure to ensure that's the case results in rejection of the patch > until it conforms. Not a problem then... resending v2. 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 0/4] iommu: Prevent oops in iommu_get() and while arch_iommu is in use
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.
This way depmod (if I'm not mistaken) can do its job, you won't be
able to remove iommu2 in the middle of execution nor install iommu
without its mach counterpart being there first, it should also fix
clients depending on this modules, e.g "modprobe bridgedriver" would
only install iommu and bridgedriver, with this new dependency iommu2
should be installed as well. BTW same happens with omap mailbox.
$ lsmod
iovmm 7225 1 bridgedriver
iommu 11084 2 bridgedriver,iovmm
iommu2 4783 1 iommu
I can send as a patch if the mailer screws the spacing, also just
copy-pasted and played with the pointers, if needed we can give better
naming.
Regards,
Omar
---
diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
index adb083e..ab2f9a9 100644
--- a/arch/arm/mach-omap2/iommu2.c
+++ b/arch/arm/mach-omap2/iommu2.c
@@ -341,15 +341,47 @@ static const struct iommu_functions omap2_iommu_ops = {
.dump_ctx = omap2_iommu_dump_ctx,
};
+/**
+ * install_iommu_arch - Install archtecure specific iommu functions
+ * @ops: a pointer to architecture specific iommu functions
+ *
+ * There are several kind of iommu algorithm(tlb, pagetable) among
+ * omap series. This interface installs such an iommu algorighm.
+ **/
+int install_iommu_arch(const struct iommu_functions **ops)
+{
+ if (*ops)
+ return -EBUSY;
+ *ops = &omap2_iommu_ops;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(install_iommu_arch);
+
+/**
+ * uninstall_iommu_arch - Uninstall archtecure specific iommu functions
+ * @ops: a pointer to architecture specific iommu functions
+ *
+ * This interface uninstalls the iommu algorighm installed previously.
+ **/
+void uninstall_iommu_arch(const struct iommu_functions **ops)
+{
+ if (*ops != &omap2_iommu_ops)
+ pr_err("%s: not your arch\n", __func__);
+
+ *ops = NULL;
+}
+EXPORT_SYMBOL_GPL(uninstall_iommu_arch);
+
static int __init omap2_iommu_init(void)
{
- return install_iommu_arch(&omap2_iommu_ops);
+ return 0;
}
module_init(omap2_iommu_init);
static void __exit omap2_iommu_exit(void)
{
- uninstall_iommu_arch(&omap2_iommu_ops);
+ /* Do nothing */
}
module_exit(omap2_iommu_exit);
diff --git a/arch/arm/plat-omap/include/plat/iommu.h
b/arch/arm/plat-omap/include/plat/iommu.h
index 174f1b9..1c8e7ee 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -177,9 +177,6 @@ extern int iommu_set_isr(const char *name,
extern void iommu_save_ctx(struct iommu *obj);
extern void iommu_restore_ctx(struct iommu *obj);
-extern int install_iommu_arch(const struct iommu_functions *ops);
-extern void uninstall_iommu_arch(const struct iommu_functions *ops);
-
extern int foreach_iommu_device(void *data,
int (*fn)(struct device *, void *));
diff --git a/arch/arm/plat-omap/include/plat/iommu2.h
b/arch/arm/plat-omap/include/plat/iommu2.h
index 10ad05f..8189f58 100644
--- a/arch/arm/plat-omap/include/plat/iommu2.h
+++ b/arch/arm/plat-omap/include/plat/iommu2.h
@@ -80,6 +80,9 @@
#define MMU_RAM_MIXED_MASK (1 << MMU_RAM_MIXED_SHIFT)
#define MMU_RAM_MIXED MMU_RAM_MIXED_MASK
+extern int install_iommu_arch(const struct iommu_functions **ops);
+extern void uninstall_iommu_arch(const struct iommu_functions **ops);
+
/*
* register accessors
*/
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index 8a51fd5..f088929 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -37,38 +37,6 @@ static struct platform_driver omap_iommu_driver;
static struct kmem_cache *iopte_cachep;
/**
- * install_iommu_arch - Install archtecure specific iommu functions
- * @ops: a pointer to architecture specific iommu functions
- *
- * There are several kind of iommu algorithm(tlb, pagetable) among
- * omap series. This interface installs such an iommu algorighm.
- **/
-int install_iommu_arch(const struct iommu_functions *ops)
-{
- if (arch_iommu)
- return -EBUSY;
-
- arch_iommu = ops;
- return 0;
-}
-EXPORT_SYMBOL_GPL(install_iommu_arch);
-
-/**
- * uninstall_iommu_arch - Uninstall archtecure specific iommu functions
- * @ops: a pointer to architecture specific iommu functions
- *
- * This interface uninstalls the
Re: [PATCH] OMAP: iovmm: fix SW flags passed by user
On Fri, Mar 25, 2011 at 12:30 PM, Sergei Shtylyov wrote: > Hello. > > Omar Ramirez Luna wrote: > >> Commit d038aee24dcd changes iovmm to receive flags specified by user, > > Please also specify that commit's summary for the human readers. Bare ID > is only usable to gitweb... Here it is the patch, introducing the change: https://patchwork.kernel.org/patch/620871/ If accepted by Hiroshi, I can update the description making reference to this patch instead of commit. 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: [RESEND PATCH] staging: tidspbridge: protect dmm_map properly
Hi, On Fri, Mar 11, 2011 at 12:31 PM, Felipe Contreras wrote: > Well, not crashing is better than crashing, regardless of the status > of the driver. And there's no reason not to include the fix. No reason indeed. > But if you send it to Greg, then it's Greg's call. Ok then. 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: [RESEND PATCH] staging: tidspbridge: protect dmm_map properly
Hi Felipe, On Fri, Mar 11, 2011 at 11:38 AM, Felipe Contreras wrote: > On Fri, Mar 11, 2011 at 7:30 PM, Felipe Contreras > wrote: >> We need to protect not only the dmm_map list, but the individual >> map_obj's, otherwise, we might be building the scatter-gather list with >> garbage. So, use the existing proc_lock for that. >> >> I observed race conditions which caused kernel panics while running >> stress tests, also, Tuomas Kulve found it happening quite often in >> Gumstix Over. This patch fixes those. >> >> Cc: Tuomas Kulve >> Signed-off-by: Felipe Contreras > > Omar, 2.6.38 is imminent, if this patch has any chance of getting in, > I think you would need to push it soon. I'll push it to my queue, however I don't think this would be even considered to make it before the official 2.6.38 which should happen in the next few days, given that it should climb to staging tree first and from there to mainline... we could attempt to shot directly to the mainline kernel, but that would raise a few eyebrows and in the end be rejected as well because this is a staging driver which is still in process of cleanup. OTOH, this patch was there long ago, since December, but I just lost the track of it at the moment, along with the other proposed solutions, so I take it as my fault for not picking it up then. 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 2/4] OMAP4: hwmod data: add mmu hwmod for ipu and dsp
Hi Benoit, On Tue, Mar 8, 2011 at 3:29 PM, Cousson, Benoit wrote: > What I meant is that the mmu does not have any explicit PRCM module enable > control. The PRCM does control only the DSP as a whole. Since hwmods are > generated with a PRCM granularity, any sub module will not be exposed by > default. Ok, got it. Thanks for the reply. > I've checked both IPU and DSP HW specs, and in the both cases, the mmu IP is > the only module with OCP port from the MPU in the relevant subsystem. > In that case, it is easier for me to remove that DSP and IPU hwmods and move > everything under the MMUs hwmods. > It will then highlight the re-use of the same IP in two different > subsystems. > > I have now to modify the OMAP4 generator in order to provide the correct > data in the right format. > > I'll try to do that before the end of this week. This will not have any real > impact for you except for the name change (mmu_ipu and mmu_dsp). > > Is that OK for you? No problem, once it is ready I'll update OMAP3 to mimic the name style. Thanks, 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] staging: tidspbridge: protect dmm_map properly
Hi Felipe, On Mon, Mar 7, 2011 at 12:02 PM, Felipe Contreras wrote: > On Mon, Dec 20, 2010 at 7:12 PM, Felipe Contreras > wrote: >> We need to protect not only the dmm_map list, but the individual >> map_obj's, otherwise, we might be building the scatter-gather list with >> garbage. So, use the existing proc_lock for that. >> >> I observed race conditions which caused kernel panics while running >> stress tests. This patch fixes those. > > I just heard that Tuomas Kulve is getting a lot of panics on Gumstix > Overo. I propose we apply this patch on the stable tree ASAP, and if > there's no better proposals, also on .38. Can you or Tuomas share the bug report data (panic log, test case maybe)? I would like to discard issues affected by timing that could be hidden with this patch. I agree that for the time being this needs to be sent upstream, even if in paper Ohad's patch solves the issue without side effects of locking. Thanks, 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 2/4] OMAP4: hwmod data: add mmu hwmod for ipu and dsp
Hi Benoit, On Mon, Mar 7, 2011 at 6:55 AM, Cousson, Benoit wrote: > Hi Omar, > > I have some concern about the introduction of a hwmod that does not match > the actual HW capability. MMU does exist, but there is no SW control for it. Maybe I'm missing something, but iommu (driver) is meant to control isp, iva, ipu and dsp MMUs; even with a simple driver interfaced with iommu, that had nothing to do with the modules mentioned, you could still deassert the reset, enable the clocks, create your tables and add entries, and so on... not that it would be useful for anybody other than the real HW containing the MMU subsystem. > In fact the only control available is for mmu + cache + logic, and that's > why the MMU is handle today under the main DSP/IPU hwmod. AFAIK, sysc configuration is missing from the old hwmods, I thought separate hwmods gave: - flexibility: so the system wouldn't dump_stack trying to read mmu registers, because the user doesn't know ipu/dsp code should handle the reset first. - clarity: so iommu handles its own mmu hwmods instead of hard coding the names of the pseudo hwmods containing the mmu. > Here you are just duplicating dsp_hwmod and ipu_hwmod with dsp_mmu_hwmod / > ipu_mmu_hwmod and adding some memory space for the mmu part. > > In that case, you can still use the previous name and add the missing > entries in it. > > The only advantage I can see is the usage of a common class that will allow > you to handle both DSP and IPU using the same "MMU" driver. > > So, what are you going to do with the remaining entries for dsp_hwmod and > ipu_hwmod? I think these can be removed, and iommu code can handle its own hwmods; but if you want to update the old ones, that can be done too, the tradeoff would be that iommu needs to know the name of the hwmods with mmu data. 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] OMAP2+: hwmod: use status bit info for reset line
Hi Paul On Fri, Mar 4, 2011 at 3:36 PM, Paul Walmsley wrote: > It's after our -rc6 cutoff, but I'll take it anyway for 2.6.39. Made a > few minor changes -- updated patch below -- if you have a chance, please > give it a quick test. The 'integration-2.6.39' branch at > git://git.pwsan.com/linux-integration now contains it. Or if you have any > comments about the changes, those are welcome too. Agree with changes, didn't see any issue either while testing. 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 v3 2/2] OMAP: IOMMU: add support to callback during fault handling
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.
>>> + /* 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.
>> 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.
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 v3 2/2] OMAP: IOMMU: add support to callback during fault handling
Hi,
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,
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.
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
> 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;
> }
> @@ -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)
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?
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 v5 3/5] OMAP4: hwmod data: add mailbox data
Hi Tony, On Thu, Feb 17, 2011 at 5:39 PM, Tony Lindgren wrote: > * Ramirez Luna, Omar [110215 16:11]: >> >> > If you have to do several platform_get_irq_byname to get this one, I'd >> > prefer to get rid of that name for OMAP4. It will make mailbox irq >> > consistent with the other hwmods. >> >> I was thinking to standardize the names to be mbox0..mboxN across all >> the platforms, reason being that the mailbox also has capabilities to >> be used not only by dsp or iva, by using a polling method. >> >> So even if the mailbox in OMAP3 is called "dsp", it has 4 more queues >> apart from the 2 used for messaging between arm and dsp, that could be >> used even if tidspbridge wasn't there. > > So what's the status of this series? Can the rest of the patches > now be applied on top of the current omap-for-linus or do the names > still need fixing? I'm going to remove the names for the hwmods with a single irq. And change the interface clock to be the main_clk, so it can be disabled when runtime pm is disabled. I won't resend the patch for OMAP4 hwmod data, as this was send earlier by Benoit. 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 v5 3/5] OMAP4: hwmod data: add mailbox data
Hi,
On Tue, Feb 15, 2011 at 4:05 PM, Cousson, Benoit wrote:
>> It is this way instead of plain platform_get_irq because omap2420 has
>> two interrupt sources to MPU and mailbox driver uses
>> platform_get_irq_byname to get the irq number.
>
> This is what I was thinking, except that on OMAP2420 the names are:
> + { .name = "dsp", .irq = 26, },
> + { .name = "iva", .irq = 34, },
>
> and on OMAP2430 and OMAP3
> + { .name = "dsp", .irq = 26, },
>
> so why is it named "mbox" on OMAP4?
I'm not very familiar with OMAP4 terminology... but IMHO, I guess
naming it dsp, would imply that this is a mailbox for the dsp, when
the interrupt can be generated by either the M3, dsp, I think even the
IVA can write into it.
> If you have to do several platform_get_irq_byname to get this one, I'd
> prefer to get rid of that name for OMAP4. It will make mailbox irq
> consistent with the other hwmods.
I was thinking to standardize the names to be mbox0..mboxN across all
the platforms, reason being that the mailbox also has capabilities to
be used not only by dsp or iva, by using a polling method.
So even if the mailbox in OMAP3 is called "dsp", it has 4 more queues
apart from the 2 used for messaging between arm and dsp, that could be
used even if tidspbridge wasn't there.
Did I get you correctly?
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 v5 3/5] OMAP4: hwmod data: add mailbox data
Hi Benoit,
On Mon, Feb 14, 2011 at 9:00 AM, Cousson, Benoit wrote:
>> +static struct omap_hwmod_irq_info omap44xx_mailbox_irqs[] = {
>> + { .name = "mbox", .irq = 26 + OMAP44XX_IRQ_GIC_START, },
>
> The original entry was unnamed since it is an unique entry and thus does not
> need to be differentiate on this platform.
>
> { .irq = 26 + OMAP44XX_IRQ_GIC_START },
>
> Do you really need to have a name here? The strategy being to provide a name
> only if more than one entry exist.
> It is perfectibility doable, I'm just trying to understand your rational.
It is this way instead of plain platform_get_irq because omap2420 has
two interrupt sources to MPU and mailbox driver uses
platform_get_irq_byname to get the irq number.
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 v5 0/5] omap: mailbox: hwmod support
Hi Kevin, Benoit, On Fri, Feb 11, 2011 at 5:01 PM, Kevin Hilman wrote: > Omar Ramirez Luna writes: > >> Mailbox hwmod support for OMAP 2,3,4. >> >> This was tested on OMAP3 (3430, 3630), minor testing >> was made on OMAP4. >> >> No testing on OMAP2 since I don't have the hardware. > > To help in testing, I wrote a simple mailbox loopback test module for > OMAP2/3/4 that I used to do send and receive messages on the MPU. This > can be used to test the mailbox without any DSP software. > > I tested it against l-o master branch and found a couple bugs in the > mailbox driver (patches posted earlier today.) > > With those patches plus my test I can send & receieve a series of > messages on the MPU, which is enough to sanity test the basic sending > and receiving messages on the MPU. > > I've tested the master branch, now it's your turn to use this test > module to validate this hmod conversion series. > > The test module is available here: > > git://gitorious.org/omap-test/mailbox.git I rechecked on OMAP3 (zoom2, zoom3) and OMAP4 (blaze), and functionality hasn't changed with the hwmod support + Kevin's mbox test. On OMAP3, although functionality is ok with this patch set, I noticed that the interface clock is not being disabled with its corresponding pm_runtime_disable call. Previously, the driver enabled/disabled the ick clock on demand, but now the clock stays enabled always (only affected by the smart-idle feature, I presume). Should this clock be placed in the main_clk field of omap_hwmod struct instead of omap_hwmod_ocp_if, I moved it to ocp_if because of the ick thing, any comment? 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 2/2] OMAP: Cleanup IOMMU error messages
Hi David,
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.
AFAIK, your patch removes this line:
- dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
Which I assume is the one being printed again in plat code, right?
> Signed-off-by: David Cohen
> ---
> arch/arm/mach-omap2/iommu2.c | 33 +--
> arch/arm/plat-omap/include/plat/iommu.h | 2 +-
> arch/arm/plat-omap/iommu.c | 36 --
> 3 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> index 14ee686..bb3d75b 100644
> --- a/arch/arm/mach-omap2/iommu2.c
> +++ b/arch/arm/mach-omap2/iommu2.c
> @@ -143,33 +143,32 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool
> on)
> __iommu_set_twl(obj, false);
> }
>
> -static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
> +static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra, u32 *iommu_errs)
> {
> - int i;
> + u32 errs = 0;
> u32 stat, da;
> - const char *err_msg[] = {
> - "tlb miss",
> - "translation fault",
> - "emulation miss",
> - "table walk fault",
> - "multi hit fault",
> - };
AFAIU, this code living in mach-omap2, means that this errors are
common for omap2,3,4 which they are. Does moving them to plat code
implies that all omap platforms expect these errors?
> stat = iommu_read_reg(obj, MMU_IRQSTATUS);
> stat &= MMU_IRQ_MASK;
> - if (!stat)
> + if (!stat) {
> + *iommu_errs = 0;
> return 0;
> + }
>
> da = iommu_read_reg(obj, MMU_FAULT_AD);
> *ra = da;
>
> - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
> -
> - for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
> - if (stat & (1 << i))
> - printk("%s ", err_msg[i]);
> - }
> - printk("\n");
> + if (stat & MMU_IRQ_TLBMISS)
> + errs |= IOMMU_ERR_TLB_MISS;
> + if (stat & MMU_IRQ_TRANSLATIONFAULT)
> + errs |= IOMMU_ERR_TRANS_FAULT;
> + if (stat & MMU_IRQ_EMUMISS)
> + errs |= IOMMU_ERR_EMU_MISS;
> + if (stat & MMU_IRQ_TABLEWALKFAULT)
> + errs |= IOMMU_ERR_TBLWALK_FAULT;
> + if (stat & MMU_IRQ_MULTIHITFAULT)
> + errs |= IOMMU_ERR_MULTIHIT_FAULT;
> + *iommu_errs = errs;
Is there any point in doing this?
Basically: *iommu_errs = stat, given that the mask values and the
error defines are the same for each case.
Besides I haven't seen two errors trigger a single interrupt.
...
>
> - iopte = iopte_offset(iopgd, da);
> -
> - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
> - __func__, da, iopgd, *iopgd, iopte, *iopte);
> + for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
> + if (errs & (1 << i))
> + printk(KERN_CONT " %s", err_msg[i]);
> + }
> + printk("\n");
I think we should get rid of the "printks".
Regards,
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/12]staging:tidspbridge - Remove hungarian notation from structures
On Mon, Jan 17, 2011 at 9:19 PM, Armando Uribe wrote: > This series of patches remove the hungarian notation from the structure's > elements. > Also the whole patch series has been rebased to the latest of tidspbridge > tree, > and minor corrections were made from the original version of Rene Sapiens. Pushed to dspbridge. 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: [PATCHv2] staging: tidspbridge: configure full L1 MMU range
Hi, On Wed, Jan 5, 2011 at 3:25 PM, Fernando Guzman Lugo wrote: > From: Guzman Lugo, Fernando > > IVA MMU can manage up to 4GB of address space through its page tables, > given that it's L1 is divided into 1MB sections it requires at least > 16KB for its table which represents 4096 entries of 32 bits each. > > Previously, only 1GB was being handled by setting the page table size > to 4KB, any virtual address beyond of the L1 size used, would fall > into memory that does not belong to L1 translation tables, leading to > unpredictable results. > > So, set the L1 table size to cover the entire MMU range (4GB) whether > is meant to be used or not. > > Reported-by: Felipe Contreras > Signed-off-by: Fernando Guzman Lugo > Signed-off-by: Felipe Contreras Nice fix, pushed to for-gkh-2.6.38 branch. 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] staging: tidspbridge - configure full L1 MMU range
Hi,
On Wed, Jan 5, 2011 at 12:20 AM, Fernando Guzman Lugo
wrote:
> Otherwise a virtual address beyond of the L1 size is used,
> the MMU hardware will look into a memory that does not belong to
> L1 translation tables. IOW; the MMU would allow to access any
> memory, configured or not.
>
How about:
IVA MMU can manage up to 4GB of address space through its page tables,
given that it's L1 is divided into 1MB sections it requires at least
16KB for its table which represents 4096 entries of 32 bits each.
Previously, only 1GB was being handled by setting the page table size
to 4KB, any virtual address beyond of the L1 size used, would fall
into memory that does not belong to L1 translation tables, leading to
unpredictable results.
So, set the L1 table size to cover the entire MMU range (4GB) whether
is meant to be used or not.
> Reported-by: Felipe Contreras
> Signed-off-by: Fernando Guzman Lugo
> Signed-off-by: Felipe Contreras
> ---
> drivers/staging/tidspbridge/core/tiomap3430.c | 6 ++
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c
> b/drivers/staging/tidspbridge/core/tiomap3430.c
> index 1be081f..ec96d1e 100644
> --- a/drivers/staging/tidspbridge/core/tiomap3430.c
> +++ b/drivers/staging/tidspbridge/core/tiomap3430.c
> @@ -70,6 +70,7 @@
> #define MMU_LARGE_PAGE_MASK 0x
> #define MMU_SMALL_PAGE_MASK 0xF000
> #define OMAP3_IVA2_BOOTADDR_MASK 0xFC00
> +#define MMU_L1_SIZE 0x4000
> #define PAGES_II_LVL_TABLE 512
> #define PHYS_TO_PAGE(phys) pfn_to_page((phys) >> PAGE_SHIFT)
>
> @@ -786,10 +787,7 @@ static int bridge_dev_create(struct bridge_dev_context
>
> pt_attrs = kzalloc(sizeof(struct pg_table_attrs), GFP_KERNEL);
> if (pt_attrs != NULL) {
> - /* Assuming that we use only DSP's memory map
> - * until 0x4000: , we would need only 1024
> - * L1 enties i.e L1 size = 4K */
> - pt_attrs->l1_size = 0x1000;
> + pt_attrs->l1_size = MMU_L1_SIZE;
How about using SZ_16K instead, struct member name already specifies
it is L1 size, is the new define needed?
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] staging: tidspbridge: remove code referred by OPT_ZERO_COPY_LOADER
Hi, On Mon, Dec 20, 2010 at 3:23 PM, Ernesto Ramos wrote: > Remove code referred by OPT_ZERO_COPY_LOADER since it is > not used. > > Signed-off-by: Ernesto Ramos > --- > drivers/staging/tidspbridge/dynload/cload.c | 60 > +-- > 1 files changed, 20 insertions(+), 40 deletions(-) Pushed to dspbridge. 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 1/5] staging:tidspbridge - Remove unused defined constants
Hi, On Thu, Dec 16, 2010 at 7:18 PM, Armando Uribe wrote: > Remove defined constants not being used. > > Signed-off-by: Armando Uribe Pushed the entire series to dspbridge. 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] dspbridge: Fix atoi to support hexadecimal numbers correctly
On Wed, Dec 22, 2010 at 7:00 PM, Ramirez Luna, Omar wrote: > On Sun, Dec 12, 2010 at 7:39 AM, Laurent Pinchart > wrote: >> For some strange reason, the DSP base image node/object properties >> description string stores hexadecimal numbers with a 'h' or 'H' suffix >> instead of a '0x' prefix. This causes parsing issue because the >> dspbridge atoi() implementation relies on strict_strtoul(), which will >> return an error because of the trailing 'h' character. >> >> As the atoi() return value is never checked for an error anyway, replace >> strict_strtoul() with simple_strtoul() to ignore the suffix. >> >> This fix gets rid of the following assertion failed messages that were >> printed when running the dsp-dummy test application. >> >> drivers/staging/tidspbridge/rmgr/nldr.c, line 1691: >> Assertion (segid == MEMINTERNALID || segid == MEMEXTERNALID) failed. >> >> Signed-off-by: Laurent Pinchart >> --- >> drivers/staging/tidspbridge/rmgr/dbdcd.c | 6 +- >> 1 files changed, 1 insertions(+), 5 deletions(-) > > This patch raises a checkpatch warning: > WARNING: consider using strict_strtoul in preference to simple_strtoul > > However on this case, simple_strtoul is preferred as explained in the commit. > > Pushed to dspbridge. BTW, fixing subject to "staging: tidspbridge" 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] dspbridge: Fix atoi to support hexadecimal numbers correctly
On Sun, Dec 12, 2010 at 7:39 AM, Laurent Pinchart wrote: > For some strange reason, the DSP base image node/object properties > description string stores hexadecimal numbers with a 'h' or 'H' suffix > instead of a '0x' prefix. This causes parsing issue because the > dspbridge atoi() implementation relies on strict_strtoul(), which will > return an error because of the trailing 'h' character. > > As the atoi() return value is never checked for an error anyway, replace > strict_strtoul() with simple_strtoul() to ignore the suffix. > > This fix gets rid of the following assertion failed messages that were > printed when running the dsp-dummy test application. > > drivers/staging/tidspbridge/rmgr/nldr.c, line 1691: > Assertion (segid == MEMINTERNALID || segid == MEMEXTERNALID) failed. > > Signed-off-by: Laurent Pinchart > --- > drivers/staging/tidspbridge/rmgr/dbdcd.c | 6 +- > 1 files changed, 1 insertions(+), 5 deletions(-) This patch raises a checkpatch warning: WARNING: consider using strict_strtoul in preference to simple_strtoul However on this case, simple_strtoul is preferred as explained in the commit. Pushed to dspbridge. Thanks, 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 2/2] staging: tidspbridge: rmgr code cleanup
Hi Ionut,
On Thu, Dec 9, 2010 at 3:47 PM, Ionut Nicu wrote:
> Reorganized some code in the rmgr module to increase
> its readability. No functional changes were done.
>
> Signed-off-by: Ionut Nicu
> ---
> drivers/staging/tidspbridge/rmgr/drv.c | 171 +---
> drivers/staging/tidspbridge/rmgr/node.c | 81 +-
> drivers/staging/tidspbridge/rmgr/rmm.c | 266 +-
> 3 files changed, 229 insertions(+), 289 deletions(-)
>
> diff --git a/drivers/staging/tidspbridge/rmgr/drv.c
> b/drivers/staging/tidspbridge/rmgr/drv.c
> index e0fc895..71a9489 100644
> --- a/drivers/staging/tidspbridge/rmgr/drv.c
> +++ b/drivers/staging/tidspbridge/rmgr/drv.c
> @@ -304,40 +304,28 @@ int drv_proc_update_strm_res(u32 num_bufs, void
> *strm_resources)
...
> @@ -608,40 +596,28 @@ int drv_request_resources(u32 dw_context, u32
> *dev_node_strg)
> DBC_REQUIRE(dw_context != 0);
> DBC_REQUIRE(dev_node_strg != NULL);
>
> - /*
> - * Allocate memory to hold the string. This will live untill
> - * it is freed in the Release resources. Update the driver object
> - * list.
> - */
> -
> if (!drv_datap || !drv_datap->drv_object)
> - status = -ENODATA;
> - else
> - pdrv_object = drv_datap->drv_object;
> -
> - if (!status) {
> - pszdev_node = kzalloc(sizeof(struct drv_ext), GFP_KERNEL);
> - if (pszdev_node) {
> - strncpy(pszdev_node->sz_string,
> - (char *)dw_context, MAXREGPATHLENGTH - 1);
> - pszdev_node->sz_string[MAXREGPATHLENGTH - 1] = '\0';
> - /* Update the Driver Object List */
> - *dev_node_strg = (u32) pszdev_node->sz_string;
> - list_add_tail(&pszdev_node->link,
> - &pdrv_object->dev_node_string);
> - } else {
> - status = -ENOMEM;
> - *dev_node_strg = 0;
> - }
> - } else {
> - dev_dbg(bridge, "%s: Failed to get Driver Object from
> Registry",
> - __func__);
> - *dev_node_strg = 0;
> - }
> + return -ENODATA;
> +
> + pdrv_object = drv_datap->drv_object;
> + *dev_node_strg = 0;
> +
> + pszdev_node = kzalloc(sizeof(struct drv_ext), GFP_KERNEL);
> + if (!pszdev_node)
> + return -ENOMEM;
> +
> + strncpy(pszdev_node->sz_string, (char *)dw_context,
> + MAXREGPATHLENGTH - 1);
> + pszdev_node->sz_string[MAXREGPATHLENGTH - 1] = '\0';
> + *dev_node_strg = (u32) pszdev_node->sz_string;
> +
> + /* Update the Driver Object List */
> + list_add_tail((struct list_head *)pszdev_node,
> + &pdrv_object->dev_node_string);
I'm not quite comfortable casting to list_head ptr, given that there
is already a member of that type, if the struct type of pszdev_node is
moved around and the first member changes, there should be issues.
Any reason to use them in this patch?
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 1/2] staging: tidspbridge: rmgr/node.c code cleanup
On Thu, Dec 9, 2010 at 3:47 PM, Ionut Nicu wrote: > Reorganized some code in rmgr/node.c to increase its > readability. Most of the changes reduce the code > indentation level and simplifiy the code. No functional > changes were done. > > Signed-off-by: Ionut Nicu > --- > drivers/staging/tidspbridge/rmgr/node.c | 607 ++ > 1 files changed, 283 insertions(+), 324 deletions(-) Pushed to dspbridge. 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] staging: tidspbridge: use the right type for list_is_last
On Mon, Dec 13, 2010 at 9:50 PM, Ramirez Luna, Omar wrote: > On Sun, Dec 12, 2010 at 6:02 AM, Laurent Pinchart > wrote: >> Hi Omar, >> >> On Wednesday 08 December 2010 23:20:23 Omar Ramirez Luna wrote: >>> Removes the following warning: >>> >>> CC [M] drivers/staging/tidspbridge/rmgr/rmm.o >>> drivers/staging/tidspbridge/rmgr/rmm.c: In function 'rmm_alloc': >>> drivers/staging/tidspbridge/rmgr/rmm.c:147: warning: passing >>> argument 1 of 'list_is_last' from incompatible pointer type >>> include/linux/list.h:170: note: expected 'const struct list_head *' >>> but argument is of type 'struct rmm_ovly_sect *' >> >> I was about to send the same patch. >> >>> Signed-off-by: Omar Ramirez Luna >> >> Acked-by: Laurent Pinchart > > Great, thanks Laurent, Ionut for the review. Pushed to dspbridge. 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 v4 5/5] OMAP: mailbox: use runtime pm for clk and sysc handling
On Fri, Dec 17, 2010 at 10:28 AM, Cousson, Benoit wrote: /* SYSCONFIG: register bit definition */ -#define AUTOIDLE (1<< 0) #define SOFTRESET (1<< 1) -#define SMARTIDLE (2<< 3) #define OMAP4_SOFTRESET (1<< 0) -#define OMAP4_NOIDLE (1<< 2) -#define OMAP4_SMARTIDLE (2<< 2) /* SYSSTATUS: register bit definition */ #define RESETDONE (1<< 0) >>> >>> Is this still required? >> >> Yes, mailbox uses the softreset every time it is requested (and that >> it has no current users at the time), it does it before configuring >> sysc, as opossed to what this patch does (where clk is enabled, sysc >> configured, and then softreset), but no harm was seen with this >> sequence. >> >> AFAIK, enabling/disabling hwmod handles prm reset but not softreset >> bit, that's why I left it there. > > In fact both are handled now. hardreset is managed for IVA, DSP and IPU, and > softreset for most the other IPs. > The only slight difference, is that softreset for the moment is just done > once at setup time. The idea being that we do not know the state the > bootloader configured IP, so we reset it by default. Yes, I noticed that softreset was handled on setup only. > I thought I did it for every enable from disable state, but in fact this is > not the case, because AFAIK there is not use-case for that yet. > > Why do you have to softreset the mailbox? That sounds like some old HW bugs > from the past that we solve using the softreset. It is adviced in the TRM that before initialization of the module, you should do a softreset and set sysc register, I found this explanation too (with git blame): omap: mailbox: Execute softreset at startup The softreset at startup is introduced as TRM describes and also some register bit definitions are added instead of magic number But mailbox doesn't execute this on init, but on every first user request, it doesn't sound like there was a bug solved with this. Also related (these were the quickest I could check): - The DSI protocol engine can be reset by software. This reset can be done for debug purposes or after a protocol error. - UART needs it too, but in this case it was executed one time at init, so hwmod was the perfect match to remove it. > Could you elaborate on that use case? If there is a need, I'd rather update > the hwmod core than letting you playing directly with the sysconfig. At > least we can expose the API we did for that purpose but didn't merge due to > the lack of any strong need. I also thought of this, but then driver would need: pm_runtime_get_sync()// to enable the clock pdata->device_reset() // to set the softreset pm_runtime_put_sync()// to disable the clock pm_runtime_get_sync()// to enable again and reconfigure sysc lost with softreset If needed I think softreset should be handled in the device enable path rather than a new API. 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 v4 5/5] OMAP: mailbox: use runtime pm for clk and sysc handling
Hi,
On Thu, Dec 16, 2010 at 10:32 AM, Kanigeri, Hari wrote:
>> @@ -130,12 +120,6 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
>> l = mbox_read_reg(MAILBOX_REVISION);
>> pr_debug("omap mailbox rev %d.%d\n", (l & 0xf0) >> 4, (l & 0x0f));
>>
>> - if (cpu_is_omap44xx())
>> - l = OMAP4_SMARTIDLE;
>> - else
>> - l = SMARTIDLE | AUTOIDLE;
>> - mbox_write_reg(l, MAILBOX_SYSCONFIG);
>> -
>
> The OMAP4 mailbox sysconfig register bits are laid out differently
> from previous OMAP mailbox's. Example is smart idle bit location is
> different from previous OMAPs. Can I know as how are you handling this
> aspect in hwmod code ?
hwmod framework provides definition for both IP models
(omap_hwmod_sysc_type1for omap2/3 or omap_hwmod_sysc_type2 for omap4),
these are located at arch/arm/mach-omap2/omap_hwmod_common_data.c
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 v4 4/5] OMAP: mailbox: build device using omap_device/omap_hwmod
On Thu, Dec 16, 2010 at 2:44 AM, Russell King - ARM Linux
wrote:
> On Thu, Dec 16, 2010 at 02:08:11PM +0530, Varadarajan, Charulatha wrote:
>> > + oh = omap_hwmod_lookup("mailbox");
>> > + if (!oh) {
>> > + pr_err("%s: unable to find hwmod\n", __func__);
>> > + return;
>> > + }
>> > +
>> > + od = omap_device_build("omap-mailbox", -1, oh,
>> > + NULL, 0,
>> > + mbox_latencies, ARRAY_SIZE(mbox_latencies),
>> > + 0);
>> > + if (!od) {
>>
>> Check for IS_ERR(od).
>>
>> > + pr_err("%s: could not build device\n", __func__);
>
> If you have an API which returns errors, and you're bothering to print
> something when an error occurs, it's often useful to print the returned
> error code, so that people can find out _why_ the error occurred.
>
Agree.
Thanks,
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 v4 5/5] OMAP: mailbox: use runtime pm for clk and sysc handling
Hi, On Thu, Dec 16, 2010 at 2:28 AM, Varadarajan, Charulatha wrote: > On Thu, Dec 16, 2010 at 12:17, Omar Ramirez Luna wrote: >> Use runtime pm APIs to enable/disable mailbox clocks and >> to configure SYSC register. >> >> Based on the patch sent by Felipe Contreras: >> https://patchwork.kernel.org/patch/101662/ >> >> Signed-off-by: Omar Ramirez Luna >> --- >> arch/arm/mach-omap2/mailbox.c | 27 +-- >> 1 files changed, 5 insertions(+), 22 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c >> index 40ddeca..f5f72ba 100644 >> --- a/arch/arm/mach-omap2/mailbox.c >> +++ b/arch/arm/mach-omap2/mailbox.c >> @@ -14,6 +14,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> >> @@ -34,12 +35,8 @@ >> #define MAILBOX_IRQ_NOTFULL(m) (1 << (2 * (m) + 1)) >> >> /* SYSCONFIG: register bit definition */ >> -#define AUTOIDLE (1 << 0) >> #define SOFTRESET (1 << 1) >> -#define SMARTIDLE (2 << 3) >> #define OMAP4_SOFTRESET (1 << 0) >> -#define OMAP4_NOIDLE (1 << 2) >> -#define OMAP4_SMARTIDLE (2 << 2) >> >> /* SYSSTATUS: register bit definition */ >> #define RESETDONE (1 << 0) > > Is this still required? Yes, mailbox uses the softreset every time it is requested (and that it has no current users at the time), it does it before configuring sysc, as opossed to what this patch does (where clk is enabled, sysc configured, and then softreset), but no harm was seen with this sequence. AFAIK, enabling/disabling hwmod handles prm reset but not softreset bit, that's why I left it there. 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 13/14] OMAP2/3: PRCM: split OMAP2/3-specific PRCM code into OMAP2/3-specific files
On Tue, Dec 14, 2010 at 10:50 PM, Paul Walmsley wrote:
> Hi,
>
> On Mon, 6 Dec 2010, Paul Walmsley wrote:
>
>> In preparation for adding OMAP4-specific PRCM accessor/mutator
>> functions, split the existing OMAP2/3 PRCM code into OMAP2/3-specific
>> files. Most of what was in mach-omap2/{cm,prm}.{c,h} has now been
>> moved into mach-omap2/{cm,prm}2xxx_3xxx.{c,h}, since it was
>> OMAP2xxx/3xxx-specific.
>>
>> This process also requires the #includes in each of these files to be
>> changed to reference the new file name. As part of doing so, add some
>> comments into plat-omap/sram.c and plat-omap/mcbsp.c, which use
>> "sideways includes", to indicate that these users of the PRM/CM includes
>> should not be doing so.
>
> This patch has been updated to also take care of getting DSPBridge to
> build again. Omar, Felipe, could you please take a look at the
> mach-omap2/dsp.c and _tiomap.h changes and ack them?
...
> diff --git a/arch/arm/mach-omap2/dsp.c b/arch/arm/mach-omap2/dsp.c
> index 6feeeae..a8b62d7 100644
> --- a/arch/arm/mach-omap2/dsp.c
> +++ b/arch/arm/mach-omap2/dsp.c
> @@ -9,11 +9,16 @@
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
> + *
> + * XXX The function pointers to the PRM/CM functions are incorrect and
> + * should be removed. No device driver should be changing PRM/CM bits
> + * directly; that's a layering violation -- those bits are the responsibility
> + * of the OMAP PM core code.
> */
>
> #include
> -#include "prm.h"
> -#include "cm.h"
> +#include "cm2xxx_3xxx.h"
> +#include "prm2xxx_3xxx.h"
> #ifdef CONFIG_BRIDGE_DVFS
> #include
> #endif
I don't have a preference, I guess part of the license header makes
them even more noticeable.
> diff --git a/drivers/staging/tidspbridge/core/_tiomap.h
> b/drivers/staging/tidspbridge/core/_tiomap.h
> index 1c1f157..7fac488 100644
> --- a/drivers/staging/tidspbridge/core/_tiomap.h
> +++ b/drivers/staging/tidspbridge/core/_tiomap.h
> @@ -21,6 +21,12 @@
>
> #include
> #include
> +/*
> + * XXX These mach-omap2/ includes are wrong and should be removed. No
> + * driver should read or write to PRM/CM registers directly; they
> + * should rely on OMAP core code to do this.
> + */
> +#include
> #include
> #include
> #include
Acked-by: Omar Ramirez Luna
Just in case someone is wondering, there is a plan to use hwmod and
move start/stop/monitor functions to dsp.c code, so, the driver can
call them through pdata.
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 1/2] staging: tidspbridge: rmgr/node.c code cleanup
Hi, On Sun, Dec 12, 2010 at 8:20 AM, Ionut Nicu wrote: >> > Are the other modes (rdma, zerocopy) not supported at all? >> >> No, those modes are not supported... the only one working for dsp >> streams is proc-copy. >> > > Is there any plan to make them work? If not, maybe the code that handles > them should be removed... I don't think there is any plan to support these modes, they have been disabled long time ago, even before the first versions of tidspbridge, I agree we should start thinking about removing them. 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] staging: tidspbridge: use the right type for list_is_last
On Sun, Dec 12, 2010 at 6:02 AM, Laurent Pinchart wrote: > Hi Omar, > > On Wednesday 08 December 2010 23:20:23 Omar Ramirez Luna wrote: >> Removes the following warning: >> >> CC [M] drivers/staging/tidspbridge/rmgr/rmm.o >> drivers/staging/tidspbridge/rmgr/rmm.c: In function 'rmm_alloc': >> drivers/staging/tidspbridge/rmgr/rmm.c:147: warning: passing >> argument 1 of 'list_is_last' from incompatible pointer type >> include/linux/list.h:170: note: expected 'const struct list_head *' >> but argument is of type 'struct rmm_ovly_sect *' > > I was about to send the same patch. > >> Signed-off-by: Omar Ramirez Luna > > Acked-by: Laurent Pinchart Great, thanks Laurent, Ionut for the review. 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] OMAP: omap_device: use pdev as parameter to get rt_va
Hi Benoit, On Mon, Dec 13, 2010 at 2:12 AM, Cousson, Benoit wrote: > Hi Omar, > > On 12/11/2010 12:45 AM, Ramirez Luna, Omar wrote: >> >> Make the parameter received by omap_device_get_mpu_rt_va >> consistent with the functions meant to be called by drivers. >> >> Also move its header declaration to appear in the set of >> functions to be used by drivers, as per the comment there. > > Please note that even if Paul submitted this API upon request from Santosh, > we do not want driver to us it most of the time. Oh, ok. Yes, I was under the impression that this ioremap was internal to hwmod, and drivers should do their own one; but then I noticed that API and since it was under the "public functions through struct platform data", I thought it was easier to pass pdev than od. > All drivers should us the generic Linux way to get physical mem resource and > then ioremap it. Then I guess this function belongs to the "public for core code" and not for drivers along with the omap_device_get_pwrdm. > I assume that if you want to update this API, you are probably already using > it. > Why cannot you use the generic way? I can leave the generic way along with ioremap, the purpose was to use omap_device APIs as much as possible. Thanks, 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] OMAP: device: make rt_va easily avaialable to drivers
On Thu, Dec 9, 2010 at 11:16 AM, Kevin Hilman
wrote:
> Omar Ramirez Luna writes:
>
>> Patch "OMAP: hwmod/device: add omap_{device, hwmod}_get_mpu_rt_va"[1],
>> introduces omap_device_get_rt_va which is meant to be called
>> by drivers to retrieve the _mpu_rt_va, however this function
>> receives a pointer to an omap_device; since there is no
>> practical way for a driver to get this parameter without
>> fiddling with pdev and container_of macro, and omap_device code
>> already does this, it is better for it to handle this case.
>>
>> Also moved header declaration to appear in the set of
>> functions to be used by drivers, as per the comment there.
>>
>> [1] http://marc.info/?l=linux-omap&m=127808467703366&w=2
>>
>> Signed-off-by: Omar Ramirez Luna
>
> Looks right, since all the other driver-accessible functions take a
> platform_device pointer, not an omap_device pointer.
>
> Acked-by: Kevin Hilman
>
> However, I thing the subject/shortlog should be clearer. IMO, this
> patch makes the API more consistent rather than making it easier.
> Also, subject prefix should be preferrably 'OMAP: omap_device:'.
I saw few patches following the convention "OMAP: device" that's why I
followed that one, I will change the subject/changelog according to
your suggestions.
Thanks,
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] staging: tidspbridge: remove file handling functions for loader
On Wed, Dec 8, 2010 at 5:49 PM, Greg KH wrote: >> BTW, I don't expect this pushed through staging yet, I need to include >> it to my branch first and then I'll send a pull request with the pile >> of patches. Sorry for the misunderstanding and thanks for the review. > > Well, don't send me patches you don't want me to apply without a big "DO > NOT APPLY" in them, otherwise I might try to :) Will do. > Then mess with the firmware symlink in userspace, don't have the driver > have to worry about it. Ok, agree. > That would be good. Again, you can put whatever firmware image you want > in that location if you wish to use different ones. That is for > userspace to do, not have the kernel worry about. Agree. 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 1/2] staging: tidspbridge: rmgr/node.c code cleanup
Hi Ionut,
On Thu, Dec 9, 2010 at 4:22 PM, Ionut Nicu wrote:
> Hi Omar,
>
> On Thu, 2010-12-09 at 23:47 +0200, Ionut Nicu wrote:
>> Reorganized some code in rmgr/node.c to increase its
>> readability. Most of the changes reduce the code
>> indentation level and simplifiy the code. No functional
>> changes were done.
>
>
>
>> /*
>> * Check stream mode. Default is STRMMODE_PROCCOPY.
>> */
>> - if (!status && pattrs) {
>> - if (pattrs->strm_mode != STRMMODE_PROCCOPY)
>> - status = -EPERM; /* illegal stream mode */
>> -
>> - }
>> - if (status)
>> - goto func_end;
>> + if (pattrs && pattrs->strm_mode != STRMMODE_PROCCOPY)
>> + return -EPERM; /* illegal stream mode */
>>
>
> While cleaning up and reviewing the code I saw this check, which unless
> it's intentional I think it looks like a bug. See comment below.
>
>
>> + strm_mode = pattrs ? pattrs->strm_mode : STRMMODE_PROCCOPY;
>> + switch (strm_mode) {
>> + case STRMMODE_RDMA:
>
> ...
>
>> +
>> + case STRMMODE_ZEROCOPY:
>
> ...
>
>> + case STRMMODE_PROCCOPY:
>
> It looks like all modes are handled here although the only one that
> could escape the validation from above is STRMMODE_PROCCOPY.
>
> I tried removing the first check and tested with the
> userspace-dspbridge strmcopy application, but I noticed the DSP hangs if
> I try to use it with anything else than the copy mode.
>
> Are the other modes (rdma, zerocopy) not supported at all?
No, those modes are not supported... the only one working for dsp
streams is proc-copy.
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 1/6] OMAP: device: make rt_va easily avaialable to drivers
On Wed, Dec 8, 2010 at 5:54 PM, Omar Ramirez Luna wrote:
> Patch "OMAP: hwmod/device: add omap_{device, hwmod}_get_mpu_rt_va"[1],
> introduces omap_device_get_rt_va which is meant to be called
> by drivers to retrieve the _mpu_rt_va, however this function
> receives a pointer to an omap_device; since there is no
> practical way for a driver to get this parameter without
> fiddling with pdev and container_of macro, and omap_device code
> already does this, it is better for it to handle this case.
>
> Also moved header declaration to appear in the set of
> functions to be used by drivers, as per the comment there.
>
> [1] http://marc.info/?l=linux-omap&m=127808467703366&w=2
>
> Signed-off-by: Omar Ramirez Luna
> ---
> arch/arm/plat-omap/include/plat/omap_device.h | 3 +--
> arch/arm/plat-omap/omap_device.c | 8 ++--
> 2 files changed, 7 insertions(+), 4 deletions(-)
This is a single patch set, please ignore the [1/6] subject :/
I'll resubmit to avoid confusions.
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] staging: tidspbridge: remove file handling functions for loader
On Wed, Dec 8, 2010 at 5:08 PM, Greg KH wrote: > On Wed, Dec 08, 2010 at 05:02:20PM -0600, Ramirez Luna, Omar wrote: >> Hi, >> >> On Wed, Dec 8, 2010 at 4:26 PM, Greg KH wrote: >> > On Tue, Dec 07, 2010 at 12:09:06AM -0600, Omar Ramirez Luna wrote: >> >> Instead use request_firmware and friends to get a valid firmware >> >> image. >> >> >> >> Right now the image is supplied dynamically through udev and the >> >> following rule: >> >> >> >> KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add", \ >> >> RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading; \ >> >> cat $FIRMWARE > /sys/$DEVPATH/data; \ >> >> echo 0 > /sys/$DEVPATH/loading'" >> > >> > Why do you need a custom firmware rule? >> >> It was meant as an example, when I compiled my minimal file system it >> didn't supply the firmware.sh script nor created /lib/firmware... I >> thought that not everybody would have the firmware.sh, so I just >> provided a sample rule. > > So, can I remove this from the changelog comment, as it's not really > needed at all? Yes it can be removed. BTW, I don't expect this pushed through staging yet, I need to include it to my branch first and then I'll send a pull request with the pile of patches. Sorry for the misunderstanding and thanks for the review. >> insmod bridgedriver.ko base_img=baseimage.dof > > Ick, why use a module parameter name at all? Why is this "special" and > different from all other firmware users? They don't have to manually > specify a file name, the driver does that. The thing is that there are N number of firmwares, e.g.: There is the official and usable firmware to play with multimedia "baseimage.dof" But there are also minimal firmwares to just ping or swap buffers with the dsp, when you just want to play around with basic features. > Please fix up the patch to not require a module parameter, distros hate > them, and users hate them even more. The driver is the one requiring the parameter (it was already this way), this patch doesn't introduce any parameter. I'll check what can be done and if nobody rejects I'll use the baseimage.dof as firmware by default. 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] staging: tidspbridge: remove file handling functions for loader
Hi, On Wed, Dec 8, 2010 at 4:26 PM, Greg KH wrote: > On Tue, Dec 07, 2010 at 12:09:06AM -0600, Omar Ramirez Luna wrote: >> Instead use request_firmware and friends to get a valid firmware >> image. >> >> Right now the image is supplied dynamically through udev and the >> following rule: >> >> KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add", \ >> RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading; \ >> cat $FIRMWARE > /sys/$DEVPATH/data; \ >> echo 0 > /sys/$DEVPATH/loading'" > > Why do you need a custom firmware rule? It was meant as an example, when I compiled my minimal file system it didn't supply the firmware.sh script nor created /lib/firmware... I thought that not everybody would have the firmware.sh, so I just provided a sample rule. > Why doesn't the default firmware loading rule that comes with udev work > properly for you? > What are you needing different here that works properly for all other drivers? firmware.sh under /lib/udev/ and dsp binaries installed under /lib/firmware/, my rule is the brute version of firmware.sh so nothing different in the script. Probably the only change would be to supply the firmware name only, as of now the insmod parameter requires the entire path, e.g.: insmod bridgedriver.ko base_img=/lib/dsp/baseimage.dof if using firmware.sh and placing firmware files under /lib/firmware/, then insmod bridgedriver.ko base_img=baseimage.dof Should be enough. 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
[v3,05/12] staging: tidspbridge: rmgr/node.c code cleanup
> Reorganized some code in rmgr/node.c to increase its
> readability. Most of the changes reduce the code
> indentation level and simplifiy the code. No functional
> changes were done.
>
> Signed-off-by: Ionut Nicu
>
> ---
> drivers/staging/tidspbridge/rmgr/node.c | 607 +++
> 1 files changed, 284 insertions(+), 323 deletions(-)
>
> diff --git a/drivers/staging/tidspbridge/rmgr/node.c
> b/drivers/staging/tidspbridge/rmgr/node.c
...
> @@ -918,170 +908,143 @@ int node_connect(struct node_object *node1, u32
> stream1,
> DBC_ASSERT(node2 != (struct node_object *)DSP_HGPPNODE);
> hnode_mgr = node2->hnode_mgr;
> }
> +
> /* Enter critical section */
> mutex_lock(&hnode_mgr->node_mgr_lock);
>
> /* Nodes must be in the allocated state */
> - if (node1_type != NODE_GPP && node_get_state(node1) != NODE_ALLOCATED)
> + if (node1_type != NODE_GPP &&
> + node_get_state(node1) != NODE_ALLOCATED) {
> status = -EBADR;
> + goto out_unlock;
> + }
>
> - if (node2_type != NODE_GPP && node_get_state(node2) != NODE_ALLOCATED)
> + if (node2_type != NODE_GPP &&
> + node_get_state(node2) != NODE_ALLOCATED) {
> status = -EBADR;
> + goto out_unlock;
> + }
>
> - if (!status) {
> - /* Check that stream indices for task and dais socket nodes
> - * are not already be used. (Device nodes checked later) */
> - if (node1_type == NODE_TASK || node1_type == NODE_DAISSOCKET) {
> - output =
> - &(node1->create_args.asa.
> - task_arg_obj.strm_out_def[stream1]);
> - if (output->sz_device != NULL)
> - status = -EISCONN;
> -
> + /* Check that stream indices for task and dais socket nodes
> + * are not already be used. (Device nodes checked later) */
Please follow the convention for multi line comments
> + if (node1_type == NODE_TASK || node1_type == NODE_DAISSOCKET) {
> + output = &(node1->create_args.asa.
> + task_arg_obj.strm_out_def[stream1]);
> + if (output->sz_device != NULL) {
can we use the simplified form?
if (output->sz_device)
> + status = -EISCONN;
> + goto out_unlock;
> }
> - if (node2_type == NODE_TASK || node2_type == NODE_DAISSOCKET) {
> - input =
> - &(node2->create_args.asa.
> - task_arg_obj.strm_in_def[stream2]);
> - if (input->sz_device != NULL)
> - status = -EISCONN;
>
> + }
> + if (node2_type == NODE_TASK || node2_type == NODE_DAISSOCKET) {
> + input = &(node2->create_args.asa.
> + task_arg_obj.strm_in_def[stream2]);
> + if (input->sz_device != NULL) {
same here
> + status = -EISCONN;
> + goto out_unlock;
> }
> +
> }
> /* Connecting two task nodes? */
> - if (!status && ((node1_type == NODE_TASK ||
> -node1_type == NODE_DAISSOCKET)
> - && (node2_type == NODE_TASK
> - || node2_type == NODE_DAISSOCKET))) {
> + if (((node1_type == NODE_TASK || node1_type == NODE_DAISSOCKET) &&
> + (node2_type == NODE_TASK ||
> + node2_type == NODE_DAISSOCKET))) {
extra set of parenthesis
> /* Find available pipe */
> pipe_id = find_first_zero_bit(hnode_mgr->pipe_map, MAXPIPES);
> if (pipe_id == MAXPIPES) {
> status = -ECONNREFUSED;
> - } else {
> - set_bit(pipe_id, hnode_mgr->pipe_map);
> - node1->outputs[stream1].type = NODECONNECT;
> - node2->inputs[stream2].type = NODECONNECT;
> - node1->outputs[stream1].dev_id = pipe_id;
> - node2->inputs[stream2].dev_id = pipe_id;
> - output->sz_device = kzalloc(PIPENAMELEN + 1,
> - GFP_KERNEL);
> - input->sz_device = kzalloc(PIPENAMELEN + 1, GFP_KERNEL);
> - if (output->sz_device == NULL ||
> - input->sz_device == NULL) {
> - /* Undo the connection */
> - kfree(output->sz_device);
> -
> - kfree(input->sz_device);
> -
> - output->sz_device = NULL;
> - input->sz_device = NULL;
> - clear_bit(pipe_id, hnode_mgr->pipe_map);
> -
