Re: [PATCH 04/10] omap_hwspinlock: Replace "hweight_long(i & 0xf) != 1" with "!is_power_of_2(i & 0xf)"
On Mon, Dec 7, 2015 at 5:03 PM, zhaoxiu.zeng wrote: > is_power_of_2 is simple, and faster than "hweightN(x) == 1" on most > architectures. Thanks. I'm not sure that speed is a major concern here, since this code executes only once during the lifetime of the driver. Readability is probably more important. -- 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 04/10] omap_hwspinlock: Replace "hweight_long(i & 0xf) != 1" with "!is_power_of_2(i & 0xf)"
Hi, On Sun, Dec 6, 2015 at 12:33 PM, Zhaoxiu Zeng wrote: > > From: Zeng Zhaoxiu > > Signed-off-by: Zeng Zhaoxiu Please explain why do you think we should make this change. Btw, the original code used is_power_of_2, but we thought hweight is more explicit so it was adopted. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remoteproc/wkup_m3: Use MODULE_DEVICE_TABLE to export alias
On Wed, Sep 16, 2015 at 3:32 PM, Dave Gerlach wrote: > Use MODULE_DEVICE_TABLE with wkup_m3_rproc_of_match so the module alias > is exported and the wkup_m3_rproc driver can automatically probe. > > Signed-off-by: Dave Gerlach Applied to remoteproc-next, thanks. -- 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
[GIT PULL] remoteproc for 4.2
The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git tags/remoteproc-4.2 for you to fetch changes up to 8de3dbd0895bebe52d069a82feae8e3fb51c1bdf: remoteproc: fix !CONFIG_OF build breakage (2015-06-18 11:44:41 +0300) - remoteproc fixes/cleanups from Suman Anna - new remoteproc TI Wakeup M3 driver from Dave Gerlach - remoteproc core support for TI's Wakeup M3 driver from both Dave and Suman - tiny remoteproc build fix from myself Dave Gerlach (3): remoteproc: introduce rproc_get_by_phandle API Documentation: dt: add bindings for TI Wakeup M3 processor remoteproc/wkup_m3: add a remoteproc driver for TI Wakeup M3 Ohad Ben-Cohen (1): remoteproc: fix !CONFIG_OF build breakage Suman Anna (4): remoteproc/ste: add blank lines after declarations remoteproc/davinci: fix quoted split string checkpatch warning remoteproc: fix various checkpatch warnings remoteproc: add a rproc ops for performing address translation Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt | 52 ++ Documentation/remoteproc.txt | 6 +++ drivers/remoteproc/Kconfig | 13 ++ drivers/remoteproc/Makefile| 1 + drivers/remoteproc/da8xx_remoteproc.c | 3 +- drivers/remoteproc/remoteproc_core.c | 115 +-- drivers/remoteproc/remoteproc_internal.h | 2 +- drivers/remoteproc/ste_modem_rproc.c | 4 +- drivers/remoteproc/wkup_m3_rproc.c | 257 + include/linux/platform_data/wkup_m3.h | 30 + include/linux/remoteproc.h | 9 ++-- 11 files changed, 460 insertions(+), 32 deletions(-) create mode 100644 Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt create mode 100644 drivers/remoteproc/wkup_m3_rproc.c create mode 100644 include/linux/platform_data/wkup_m3.h -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] hwspinlock for 4.2
The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock.git tags/hwspinlock-4.2 for you to fetch changes up to bd5717a4632cdecafe82d03de7dcb3b1876e2828: hwspinlock: qcom: Correct msb in regmap_field (2015-07-01 16:15:05 +0300) - hwspinlock core DT support from Suman Anna - OMAP hwspinlock DT support from Suman Anna - QCOM hwspinlock DT support from Bjorn Andersson - a new CSR atlas7 hwspinlock driver from Wei Chen - CSR atlas7 hwspinlock DT binding document from Wei Chen - a tiny QCOM hwspinlock driver fix from Bjorn Andersson Bjorn Andersson (3): DT: hwspinlock: Add binding documentation for Qualcomm hwmutex hwspinlock: qcom: Add support for Qualcomm HW Mutex block hwspinlock: qcom: Correct msb in regmap_field Suman Anna (4): Documentation: dt: add common bindings for hwspinlock hwspinlock/core: add device tree support Documentation: dt: add the omap hwspinlock bindings document hwspinlock/omap: add support for dt nodes Wei Chen (2): hwspinlock: add a CSR atlas7 driver DT: hwspinlock: add the CSR atlas7 hwspinlock bindings document Documentation/devicetree/bindings/hwlock/hwlock.txt | 59 +++ Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt | 26 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 +++ Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt | 28 + Documentation/hwspinlock.txt | 10 ++ MAINTAINERS | 1 - arch/arm/mach-omap2/Makefile | 3 -- arch/arm/mach-omap2/hwspinlock.c | 60 drivers/hwspinlock/Kconfig | 24 +++ drivers/hwspinlock/Makefile | 2 ++ drivers/hwspinlock/hwspinlock_core.c | 79 +++ drivers/hwspinlock/omap_hwspinlock.c | 18 --- drivers/hwspinlock/qcom_hwspinlock.c | 181 +++ drivers/hwspinlock/sirf_hwspinlock.c | 136 include/linux/hwspinlock.h | 7 + 15 files changed, 605 insertions(+), 68 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt create mode 100644 Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c create mode 100644 drivers/hwspinlock/qcom_hwspinlock.c create mode 100644 drivers/hwspinlock/sirf_hwspinlock.c -- 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 1/4] remoteproc: introduce rproc_get_by_phandle API
On Thu, Jun 18, 2015 at 7:01 PM, Dave Gerlach wrote: > Oh sorry about. Pulled your for-next, tried it out, everything looks good to > me, > thanks! Thanks! -- 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 1/4] remoteproc: introduce rproc_get_by_phandle API
Hi Dave,
On Wed, Jun 17, 2015 at 9:46 PM, Dave Gerlach wrote:
> Allow users of remoteproc the ability to get a handle to an rproc by
> passing a phandle supplied in the user's device tree node. This is
> useful in situations that require manual booting of the rproc.
>
> This patch uses the code removed by commit 40e575b1d0b3 ("remoteproc:
> remove the get_by_name/put API") for the ref counting but is modified
> to use a simple list and locking mechanism and has rproc_get_by_name
> replaced with an rproc_get_by_phandle API.
>
> Signed-off-by: Dave Gerlach
> Signed-off-by: Suman Anna
> ---
> v4->v5: Fixed build error from of_find_node_by_phandle when !CONFIG_OF
> based on offline discussion.
We can't rebase our for-next branch, so I've applied a
similar fix in a separate patch.
Please check our for-next branch to make sure things still work for you.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] remoteproc: Introduce wkup_m3_rproc driver
On Fri, May 22, 2015 at 11:45 PM, Dave Gerlach wrote: > Hi, > This patch series is v4 of the series to add a wkup_m3_rproc driver > for TI AM335xi and AM437x SoCs. This family of SoCs contains an ARM > Cortex M3 coprocessor that is responsible for low-level power tasks > that cannot be handled by the main ARM Cortex A8 so firmware running > from the CM3 can be used instead. This driver handles loading > of the firmware and reset of the CM3. Change info can be found > with each patch. Applied all 4 patches to remoteproc's for-next branch. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] remoteproc: add a rproc ops for performing address translation
Hi Dave, On Wed, Apr 1, 2015 at 10:37 PM, Dave Gerlach wrote: > From: Suman Anna > > The rproc_da_to_va API is currently used to perform any device to > kernel address translations to meet the different needs of the remoteproc > core/drivers (eg: loading). The functionality is achieved within the > remoteproc core, and is limited only for carveouts allocated within the > core. > > A new rproc ops, da_to_va, is added to provide flexibility to platform > implementations to perform the address translation themselves when the > above conditions cannot be met by the implementations. The rproc_da_to_va() > API is extended to invoke this ops if present, and fallback to regular > processing if the platform implementation cannot provide the translation. > This will allow any remoteproc implementations to translate addresses for > dedicated memories like internal memories. Can you please provide specific examples where this is needed and how it is going to be used? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] remoteproc for 4.1
The following changes since commit c517d838eb7d07bbe9507871fab3931deccff539: Linux 4.0-rc1 (2015-02-22 18:21:14 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git tags/remoteproc-4.1-next for you to fetch changes up to 315491e5d6ee66838a18a8ca0c14e6ffb376e48c: remoteproc: add IOMMU hardware capability flag (2015-03-12 10:43:26 +0200) Suman Anna is adding remoteproc support for processors not behind IOMMUs. Suman Anna (1): remoteproc: add IOMMU hardware capability flag drivers/remoteproc/da8xx_remoteproc.c | 1 + drivers/remoteproc/omap_remoteproc.c | 2 ++ drivers/remoteproc/remoteproc_core.c | 15 ++- drivers/remoteproc/ste_modem_rproc.c | 1 + include/linux/remoteproc.h| 2 ++ 5 files changed, 8 insertions(+), 13 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: [PATCH v8 0/4] hwspinlock core & omap dt support
On Thu, Apr 16, 2015 at 12:31 PM, Mark Rutland wrote: > Hi, > > Apologies for the delay on this. > >> > Gentle reminder. Can you please provide your ack on the bindings in this >> > series (Patches 1 & 3) for Ohad to queue up the series for 4.1. >> > >> >> Ping again, if you can provide your ack on these bindings and the qcom >> hwmutex bindings, then Ohad can pick up both the series for 4.1. Both >> series are blocked on getting the binding acks. > > Both the bindings look sane to me, so for patches 1 and 3: > > Acked-by: Mark Rutland Great, thanks a lot Mark! -- 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 v8 0/4] hwspinlock core & omap dt support
Hi Suman, On Thu, Mar 5, 2015 at 4:01 AM, Suman Anna wrote: > This is the latest version of the hwspinlock dt support series, > rebased onto v4.0-rc1 and addressing the long discussion on the > bindings in v7 [1]. I really hope that this series can make it > into 4.1. >From a quick glance this looks great, thanks! > Mark, > Can you please provide your Acked-by for the binding documents > so that Ohad can pick up the patches for the next merge window? That would be perfect. Once we'll have it I could move forward with this towards 4.1. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] remoteproc: use a flag to detect the presence of IOMMU
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna wrote: > The remoteproc driver core currently relies on iommu_present() on > the bus the device is on, to perform MMU management. However, this > logic doesn't scale for multi-arch, especially for processors that > do not have an IOMMU. Replace this logic instead by using a h/w > capability flag for the presence of IOMMU in the rproc structure. > > This issue is seen on OMAP platforms when trying to add a remoteproc > driver for a small Cortex M3 called the WkupM3 used for suspend / > resume management on TI AM335/AM437x SoCs. This processor does not > have an MMU. Same is the case with another processor subsystem > PRU-ICSS on AM335/AM437x. All these are platform devices, and the > current iommu_present check will not scale for the same kernel image > to support OMAP4/OMAP5 and AM335/AM437x. > > The existing platform implementation drivers - OMAP remoteproc, STE > Modem remoteproc and DA8xx remoteproc, are updated as well to properly > configure the newly added rproc field. > > Cc: Robert Tivy > Cc: Linus Walleij > Signed-off-by: Suman Anna Applied to remoteproc's for-next branch. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] remoteproc: add support to handle internal memories
On Thu, Feb 12, 2015 at 10:54 PM, Suman Anna wrote: > My original motivation was that it would only need to be added on > firmwares requiring support for loading into internal memories, > otherwise, these are something left to be managed by the software > running on the remote processor completely, and MPU will not even touch > them. Sure. But even if you guys will use this interface correctly, this patch essentially exposes ioremap to user space, which is something we generally want to avoid. > So, let me know if this is a NAK. If so, we have two options - one to go > the sram node model where each of them have to be defined separately, > and have a specific property in the rproc nodes to be able to get the > gen_pool handles. The other one is simply to define these as and > use devm_ioremap_resource() (so use DT for defining the regions instead > of a resource table entry). Any approach where these regions are defined explicitly really sounds better. If you could look into these two alternatives that would be great. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] remoteproc: add support to handle internal memories
On Wed, Feb 11, 2015 at 10:57 PM, Tony Lindgren wrote:
>> > +static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
>> > *rsc,
>> > + int offset, int avail)
>> > +{
>> ...
>> > + va = (__force void *)ioremap_nocache(rsc->pa, rsc->len);
>>
>> Back in the days when we developed remoteproc there was a tremendous
>> effort to move away from ioremap when not strictly needed.
>
> The use of ioremap in general is just fine for drivers as long
> as they access a dedicated area to the specific device. Accessing
> random registers and memory in the SoC is what I'm worried about.
Yes, the proposed interface essentially allows exactly this random
access, since the parameters to ioremap would be provided from the
user space (specifically from the resource table contained within the
firmware of the remote processor).
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On Fri, Feb 6, 2015 at 2:34 AM, Bjorn Andersson wrote: >> Yep, will do as soon as I hear from Ohad on what to do with the patch >> "hwspinlock/core: maintain a list of registered hwspinlock banks" that I >> dropped from v7. Without that and dropping hwlock-base-id, I can't get >> the translations done. >> > > My suggestion is to replace the global id-tree with a list of hwlocks > and iterate over these if we ever get more than one driver registered. > As long as #hwlock-drivers < log(#total-hwlocks) this should be > faster. > > I would however argue that clients that would notice any kind of > difference are using the API incorrectly. > > Unfortunately this is a somewhat larger change than just slapping DT > support on the framework :/ I suspect we want to wait with framework changes until there's a real hardware use case justifying them. With regard to DT, however, we obviously do want to be prepared for multiple hwlock blocks even if they do not exist today. So how about adopting an approach where: - DT fully supports multi hwlock blocks (i.e. no global id field) - Framework left mostly unchanged at the moment. This means issuing an explicit error in case a secondary hwlock block shows up, and then hwlock id could trivially be the lock index. If multi hwlock hardware use case, or some new hwlock id translation requirements, do show up in the future, it's always easy to add framework support for it. At that point we'll know better what the requirements are, and framework changes would be justifiable. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] remoteproc: add support to handle internal memories
Hi Suman,
On Fri, Jan 9, 2015 at 11:21 PM, Suman Anna wrote:
> A remote processor may need to load certain firmware sections into
> internal memories (eg: RAM at L1 or L2 levels) for performance or
> other reasons. Introduce a new resource type (RSC_INTMEM) and add
> an associated handler function to handle such memories. The handler
> creates a kernel mapping for the resource's 'pa' (physical address).
...
> + * rproc_handle_intmem() - handle internal memory resource entry
> + * @rproc: rproc handle
> + * @rsc: the intmem resource entry
> + * @offset: offset of the resource data in resource table
> + * @avail: size of available data (for image validation)
> + *
> + * This function will handle firmware requests for mapping a memory region
> + * internal to a remote processor into kernel. It neither allocates any
> + * physical pages, nor performs any iommu mapping, as this resource entry
> + * is primarily used for representing physical internal memories. If the
> + * internal memory region can only be accessed through an iommu, please
> + * use a devmem resource entry.
> + *
> + * These resource entries should be grouped near the carveout entries in
> + * the firmware's resource table, as other firmware entries might request
> + * placing other data objects inside these memory regions (e.g. data/code
> + * segments, trace resource entries, ...).
> + */
> +static int rproc_handle_intmem(struct rproc *rproc, struct fw_rsc_intmem
> *rsc,
> + int offset, int avail)
> +{
...
> + va = (__force void *)ioremap_nocache(rsc->pa, rsc->len);
Back in the days when we developed remoteproc there was a tremendous
effort to move away from ioremap when not strictly needed.
I'd be happy if someone intimate with the related hardware could ack
that in this specific case ioremap is indeed needed. No need to review
the entire patch, or anything remoteproc, just make sure that
generally ioremap is how we want to access this internal memory.
Tony or Kevin any chance you could take a look and ack?
If ioremap is indeed the way to go, I'd also expect that we wouldn't
have to use __force here, but that's probably a minor patch cleanup.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] couple of generic remoteproc enhancements
Hi Suman, On Tue, Feb 3, 2015 at 10:55 PM, Suman Anna wrote: > > On 01/09/2015 03:21 PM, Suman Anna wrote: > >> Hi Ohad, > >> > >> The following is an updated patchset addressing the previous pending > >> comments > >> from v1 & v2, and are rebased onto the latest 3.19-rc3 (are rc independent > >> actually). > >> > >> The patches are mainly developed to support the WkupM3 remote processor > >> driver > >> on TI AM335x/AM437x SoCs, and I have verified the loading using the latest > >> version of Dave's WkupM3 remoteproc work [1] > >> > >> The only change in v3 is on the second patch, it mainly leverages the > >> memcpy_toio and memset_io functions for copying/loading code into the > >> internal memory sections. An additional argument has to be added to the > >> rproc_da_to_va function to make this distinction. > > > > Any comments on this series, or can I assume that this will make it to > > v3.20? > > ping, want to make sure this has not fallen off your radar.. It has not. I'm a bit swamped so apologies for not taking care of this faster. I believe I'll get to it next week. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On Sat, Jan 31, 2015 at 7:41 AM, Ohad Ben-Cohen wrote: > On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson wrote: > > In a system where you have two hwlock blocks lckA and lckB, each > > consisting of 8 locks and you have dspB that can only access lckB > > This is a good example - thanks. To be able to cope with such cases we > will have to pass a hwlock block reference and its relative lock id. Additionally, to support such a scenario, we can no longer retain the simple dynamic allocation API we have today, because it might end up allocating dspB an hwlock from IckA. We will have to make sure hwlocks are allocated only from pools visible to the user, something that will change not only the hwspinlock API but also the way it maintains the hwlocks. I suspect we want to wait for such hardware to show up first, and only then add framework support for it. Regardless, we obviously do want to make sure our DT binding is prepared for the worse, so we'll drop the "base-id" field. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson wrote: > In a system where you have two hwlock blocks lckA and lckB, each > consisting of 8 locks and you have dspB that can only access lckB This is a good example - thanks. To be able to cope with such cases we will have to pass a hwlock block reference and its relative lock id. The DT binding should definitely be prepared for such cases (just kill the base-id field?), but let's see what it means about the Linux implementation. Since the existence of several hwblocks is still fictional (Bjorn, please confirm too?), we may prefer to introduce changes to support it only when it shows up; it all depends on the amount of changes needed. Suman, care to take a look please? >> - Sometimes a remote processor, which may not be running Linux, will >> have to dynamically allocate a hwlock, and send the ID of the >> allocated lock to us (another processor running Linux) >> > I'm sorry but you cannot have a system on both sides that is allowed > to do dynamic allocation from a limited set of resources. Of course not. On such systems, Linux is not the one responsible for allocating the hwlocks, at least not during part of the time or from part of the hwlocks. There were a few different use cases, with different semantics, that required communicating to Linux an hwlock id, but since none of them have reached mainline, we should only remember they may show up one day, but not put too much effort to support them right now. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On Tue, Jan 20, 2015 at 8:05 PM, Tony Lindgren wrote: > How about default to Linux id space and allow overriding that with > a module param option if needed? I'm not sure I'm following. If the main point of contention is the base_id field, I'm also fine with removing it entirely, as I'm not aware of any actual user for it (Suman please confirm?). Mark? Rob? Will you accept Suman's patches if the base_id field is removed? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
Mark, On Fri, Jan 16, 2015 at 12:17 PM, Mark Rutland wrote: >> The hwlock is a basic hardware primitive that allow synchronization >> between different processors in the system, which may be running Linux >> as well as other operating systems, and may have no other means of >> communication. >> >> The hwlock id numbers are predefined, global and static across the >> entire system: Linux may boot well after other operating systems are >> already running and using these hwlocks to communicate, and therefore, >> in order to use these hardware devices, it must not enumerate them >> differently than the rest of the system. > > That's not true. > > In order to communicate it must agree with the other users as to the > meaning of each instance, and the protocol for use. That doesn't > necessarily mean that Linux needs to know the numerical ID from a > datasheet, and regardless that ID is separate from the logical ID Linux > uses internally. Let me describe hwspinlocks a bit more so we all get to know it better and can then agree on a proper solution. - What makes handling of hwspinlock ID numbers convenient is the fact that it's not based on random datasheet numbers. In fact, hwspinlocks is just special memory: usually datasheets just define the base address and the size of the hwspinlock area. So any numerical ID we use to call the locks themselves are already logical and sane, similar to the way we handle memory (i.e. if we have 32 locks we'll always use 0..31). So hwlocks ids are very much like memory addressing, and not irq numbers. - Sometimes Linux will have to dynamically allocate a hwlock, and send the ID of the allocated lock to a remote processor (which may not be running Linux). - Sometimes a remote processor, which may not be running Linux, will have to dynamically allocate a hwlock, and send the ID of the allocated lock to us (another processor running Linux) We cannot tell in advance what kind of IPC is going to be used for sending and receiving this hwlock ID. Some are handled by Linux (kernel) and some by the user space. So we must be able to expose an ID the system will understand as well as receive one. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock
On Thu, Jan 15, 2015 at 4:42 PM, Rob Herring wrote: > On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland wrote: >> On Thu, Jan 15, 2015 at 01:52:01PM +, Mark Rutland wrote: >>> On Wed, Jan 14, 2015 at 08:58:18PM +, Suman Anna wrote: >>> > +- hwlock-base-id: An unique base Id for the locks for a particular >>> > hwlock >>> > + device. This property is mandatory for all platform >>> > + implementations. >>> >>> This property makes no sense. The ID encoded in the hwlock cells is >>> relative to the instance (identified by phandle), not global. So the DT >>> has no global ID space. >>> >>> Why do you think you need this? >> >> Having looked at the way this proeprty is used, NAK. >> >> If you need to carve up a Linux-internal ID space, do that dynamically. >> There is no need for this property. > > Better yet, don't create a Linux ID space for this. Everywhere we have > one, we want to get rid of it. Rob, Mark, The hwlock is a basic hardware primitive that allow synchronization between different processors in the system, which may be running Linux as well as other operating systems, and may have no other means of communication. The hwlock id numbers are predefined, global and static across the entire system: Linux may boot well after other operating systems are already running and using these hwlocks to communicate, and therefore, in order to use these hardware devices, it must not enumerate them differently than the rest of the system. Given that these id numbers are global, system-wide, static and predefined, where Linux may just be one user of them, please reconsider the approach as implemented by Suman, or suggest an alternative one you may prefer. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/4] hwspinlock core & omap dt support
Hi Suman, On Wed, Jan 14, 2015 at 10:58 PM, Suman Anna wrote: > This is an updated version of the hwspinlock dt support series, > rebased onto v3.19-rc3 and mainly addresses the continued discussion > on the need to maintain a list of registered spinlock banks [1]. > I have removed this patch as per your wish, and as a result the > burden of the spinlock validation and deferred probing is pushed > onto the hwspinlock clients. Sorry for the delay in refreshing this > series, hopefully this can be the last revision. > > Following are the main changes in v7 w.r.t v6: > - Dropped the patch "hwspinlock/core: maintain a list of registered > hwspinlock banks" > - Updated generic hwspinlock bindings to make hwlock-base-id property > mandatory. > - Updated the OMAP hwspinlock binding and DT support patches to correct > for the mandatory hwlock-base-id property. > - Updated the common OF helpers patch to move the of_hwspin_lock_get_base_id() > and of_hwspin_lock_get_num_locks() functions into the internal header, > these are no longer exported, but available for platform implementations. > of_hwspin_lock_get_id() is also simplified now. This looks good, thanks. Please try to get an ACK from a DT maintainer on the first two dt patches, and then I can take the patches to Linus. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] rpmsg for 3.19
[Resending in plain text mode - sorry] The following changes since commit 5d01410fe4d92081f349b013a2e7a95429e4f2c9: Linux 3.18-rc6 (2014-11-23 15:25:20 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/rpmsg.git tags/rpmsg-3.19-next for you to fetch changes up to b1b9891441fa33fd0d49b5cb3aa7f04bca1cc1db: rpmsg: use less buffers when vrings are small (2014-11-26 18:24:36 +0200) A single patch from Suman Anna which makes rpmsg use less buffers when small vrings are being used. Suman Anna (1): rpmsg: use less buffers when vrings are small drivers/rpmsg/virtio_rpmsg_bus.c | 44 +++- 1 file changed, 30 insertions(+), 14 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: [PATCHv2] rpmsg: compute number of buffers to allocate from vrings
On Thu, Nov 27, 2014 at 12:30 AM, Suman Anna wrote: > Yep, I have reviewed and verified the changes, it is good to go. Applied, thanks! -- 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] rpmsg: compute number of buffers to allocate from vrings
Hi Suman,
On Thu, Nov 13, 2014 at 7:46 PM, Suman Anna wrote:
> Hi Ohad,
>
> On 09/16/2014 01:33 PM, Suman Anna wrote:
>> The buffers to be used for communication are allocated during the
>> rpmsg virtio driver's probe, and the total number of buffers is
>> currently hard-coded to 512. The vring configuration can vary from
>> one platform to another or between different remote processors. The
>> setup of the receive buffers will throw a WARN_ON if the associated
>> vrings are configured with less than 256 buffers (in each direction).
>> So, adjust this hard-coded value to rely on the number of buffers the
>> virtqueue vring is setup with, but also limit to use 256 buffers at
>> most in each direction to avoid wacky resource tables consuming up
>> unreasonable memory.
>>
>> NOTE: The number of buffers is already assumed to be symmetrical
>> in each direction, and that logic is not unchanged.
>>
>> Signed-off-by: Suman Anna
>> ---
>> v2 changes:
>> - add upper limit on buffers and update comments
>> - revise patch description
>
> Any comments on this one, if not can you pick this up for 3.19?
Did some small changes - untested, not even compiled - can you please
make sure it works for you?
Thanks,
Ohad.
From b1b9891441fa33fd0d49b5cb3aa7f04bca1cc1db Mon Sep 17 00:00:00 2001
From: Suman Anna
Date: Tue, 16 Sep 2014 13:33:07 -0500
Subject: [PATCH] rpmsg: use less buffers when vrings are small
Adjust the number of rpmsg buffers to rely on the size of the
vring, instead of using the hard coded value of 512 (256 per
direction).
This is needed when small vrings are being used, where 256
buffers are too much to fit in a vring.
While considering the vring size, keep using the 512 hard coded
value as an upper limit to avoid wacky resource tables consuming
unreasonable amount of memory.
NOTE: The number of buffers is already assumed to be symmetrical
in each direction, and that logic is unchanged.
Signed-off-by: Suman Anna
[edit commit message, small code and comment simplification]
Signed-off-by: Ohad Ben-Cohen
---
drivers/rpmsg/virtio_rpmsg_bus.c | 44 +++-
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index b6135d4..92f6af6 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -41,6 +41,7 @@
* @svq: tx virtqueue
* @rbufs: kernel address of rx buffers
* @sbufs: kernel address of tx buffers
+ * @num_bufs: total number of buffers for rx and tx
* @last_sbuf: index of last tx buffer used
* @bufs_dma: dma base addr of the buffers
* @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders.
@@ -60,6 +61,7 @@ struct virtproc_info {
struct virtio_device *vdev;
struct virtqueue *rvq, *svq;
void *rbufs, *sbufs;
+ unsigned int num_bufs;
int last_sbuf;
dma_addr_t bufs_dma;
struct mutex tx_lock;
@@ -86,13 +88,14 @@ struct rpmsg_channel_info {
#define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv)
/*
- * We're allocating 512 buffers of 512 bytes for communications, and then
- * using the first 256 buffers for RX, and the last 256 buffers for TX.
+ * We're allocating buffers of 512 bytes each for communications. The
+ * number of buffers will be computed from the number of buffers supported
+ * by the vring, upto a maximum of 512 buffers (256 in each direction).
*
* Each buffer will have 16 bytes for the msg header and 496 bytes for
* the payload.
*
- * This will require a total space of 256KB for the buffers.
+ * This will utilize a maximum total space of 256KB for the buffers.
*
* We might also want to add support for user-provided buffers in time.
* This will allow bigger buffer size flexibility, and can also be used
@@ -102,9 +105,8 @@ struct rpmsg_channel_info {
* can change this without changing anything in the firmware of the remote
* processor.
*/
-#define RPMSG_NUM_BUFS (512)
+#define MAX_RPMSG_NUM_BUFS (512)
#define RPMSG_BUF_SIZE (512)
-#define RPMSG_TOTAL_BUF_SPACE (RPMSG_NUM_BUFS * RPMSG_BUF_SIZE)
/*
* Local addresses are dynamically allocated on-demand.
@@ -579,7 +581,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
* either pick the next unused tx buffer
* (half of our buffers are used for sending messages)
*/
- if (vrp->last_sbuf < RPMSG_NUM_BUFS / 2)
+ if (vrp->last_sbuf < vrp->num_bufs / 2)
ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++;
/* or recycle a used one */
else
@@ -948,6 +950,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
struct virtproc_info *vrp;
void *bufs_va;
int err = 0, i;
+ size_t total_buf_space;
vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
if (!vrp)
@@ -968,10 +971,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
vrp->rvq = v
Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes
Hi Bjorn, On Thu, Nov 20, 2014 at 2:43 AM, Bjorn Andersson wrote: > I still have a huge problem understanding the awesomeness with the > "base_id". If you have a SoC with 2 hwlock blocks; say 8+8 locks, used > for interaction with e.g. a modem and a video core respectively. > Why would you in either remote system offset the locks with 8? > Wouldn't e.g the modem use locks hwlock0:0-7 and video core use locks > hwlock1:0-7? Please see below > What systems use more than one hwlock block None that we know of. This concern was raised some time ago by Arnd and since it was easy to deal with we added the 'base_id' notion. > and do you know of any reasons why these hwlocks are globally numbered? Because on an heterogeneous asymmetric multiprocessing systems you sometimes have use cases where hwlocks are dynamically allocated and their id numbers need to be synchronized between user space applications, the Linux kernel, and entities running on remote processors (which are likely not running Linux). For this to work we have to have some system-wide global hwlocks naming that is predefined and known to all processors. A mere number seems like the simplest naming method. A dynamic hwlock request API could return "hwlock1:0" but a mere 8 seems easier to deal with. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers
On Fri, Nov 14, 2014 at 7:09 PM, Suman Anna wrote: >> It seems to me that hwspin_lock_request_specific failures should be >> used by clients to defer their probing. Why wouldn't such a simple >> solution work? > Because the API always returns NULL on failures and there is no way for > the clients to figure out if the lock id passed is invalid or the bank > containing the lock is not registered. It seems to me this may always be the case - lock ids may be wrong and there's no way to fully validate them. Let's start with the simpler approach where hwspin_lock_request_specific failures are used by clients to defer their probing. If a real use case will require changes we can always do that later, though it seems to me the only gain by changing this API is to catch a small subset of fatal DT mistakes which will anyway be caught very early in the development. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers
Hi Suman,
On Thu, Nov 13, 2014 at 11:02 PM, Suman Anna wrote:
> OK, lets take an example. I have say 2 device instances, say
> hwlock1: hwlock@0 {
> hwlock-num-locks = <32>
> hwlock-base-id = <0>;
> #hwlock-cells = <1>;
> };
> hwlock2: hwlock@0 {
> hwlock-num-locks = <32>
> hwlock-base-id = <32>;
> #hwlock-cells = <1>;
> };
>
> some-client {
> hwlocks = <&hwlock1 32>, <&hwlock2 0>;
> };
>
> The first args value is incorrect, and I expect the API to return an
> error. I don't think the API can make assumptions that all passed in
> values will be correct. What do you suggest that the API do here?
I think this is just one example of many potential mistakes the DT
developer can have, and that there's nothing really special about it.
What if the '5' digit below is a typo, and the actual hardware
assignment was supposed to be '4' ?
some-client {
hwlocks = <&hwlock1 5>, <&hwlock2 5>;
};
I don't see how much different this mistake is from the one you
mentioned above. There's a limit to how much we can really catch DT
mistakes in the code, just like we couldn't really catch past mistakes
in the platform data structures.
If we can easily add some validations then great, but if we have to go
to great lengths just to gain very limited validations, then I'd vote
against doing so.
> One solution to handle #1 is to also make the hwlock-num-locks property
> also mandatory (even though it could have been read from a register
Yeah, this is awkward. We shouldn't impose this on implementations
like OMAP which don't need it.
Again I think for the very limited validation this buys us - it's not worth it.
> And, how do you propose we solve the problem of deferred probing?
It seems to me that hwspin_lock_request_specific failures should be
used by clients to defer their probing. Why wouldn't such a simple
solution work?
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Nov 13, 2014 at 7:38 PM, Suman Anna wrote: > No, not always, because, either of them can be optional between > different platform implementations. For example, on OMAP, the number of > locks is read from an IP register, and not coded in DT. Similarly, > base_id can be optional on SoCs that don't have multiple IP instances. It can, but base_id isn't optional today --- it must be provided via platform_data --- and I'm not sure we should implicitly change this semantics while moving to DT. We never guessed the base_id before, even if there was only a single IP instance, and I'm not convinced we should start doing it now. About the number of locks - why do we even need it at this point? the global lock id should just be base_id + the lock index. > IMHO, this life cycle management is not that complicated It's always very easy to add code, really. It is never complicated. But then gradually the code becomes harder to maintain, debug, and change. On the contrary, achieving the same functionality with less code is always harder. But when we get there, the results are pretty satisfying. In this case I still feel the extra linked list is redundant. Please try it: ditch the "hwspinlock/core: maintain a list of registered hwspinlock banks" patch, and replace of_hwspin_lock_get_id with a slim method that just returns the base_id + the lock index. Let me know if it works for you. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers
Hi Suman,
On Wed, Nov 12, 2014 at 9:32 PM, Suman Anna wrote:
>> Is this the validation you mentioned which requires the existence of
>> "hwspinlock/core: maintain a list of registered hwspinlock banks" ?
>
> Well, not exactly. The validation is on the following segment,
>
> + id = of_hwspin_lock_simple_xlate(bank, &args);
> + if (id < 0 || id >= bank->num_locks) {
> + ret = -EINVAL;
> + goto out;
> + }
I'm not entirely convinced that this justifies adding the proposed
linked list. Can't we can get the base_id and number of locks by
traversing the DT?
> That said, it is also needed to provide the support for deferred probing
> without changing the return code conventions on the existing API.
Here too, adding a data structure maintaining memory objects and
taking care of it life cycle seems a bit overkill for anything to do
with return values.
> Yes, and wouldn't that require that the id is validated? It just cannot
> return any return value, and expect it will be verified somewhere else
> or in a following API call. Not doing the validation unnecessarily
> complicates the system usage of a lock as you are sharing an invalid
> lock to a remote processor and then you have two validation failure
> paths on different processors.
Validation is great but I'm still not convinced it can't be done
without maintaining another data structure.
Please show me specific technical issues that can't be resolved
without adding the proposed linked list.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes
Hi Suman, On Wed, Nov 12, 2014 at 9:50 PM, Suman Anna wrote: > None of the OMAPs have multiple IP instances, and as such the base-id is > an optional property. I have made this change to make sure we atleast > attempt to use the value if mentioned in DT and not hard-coding the > value to begin with (going by the optional property semantics). If and > when multiple instances get added and a secondary node doesn't add the > property, the node will not be registered with the core due to an > overlap failure. Ok, that sounds good. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes
Hi Suman,
On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna wrote:
> static int omap_hwspinlock_probe(struct platform_device *pdev)
> {
> - struct hwspinlock_pdata *pdata = pdev->dev.platform_data;
> + struct device_node *node = pdev->dev.of_node;
> struct hwspinlock_device *bank;
> struct hwspinlock *hwlock;
> struct resource *res;
> void __iomem *io_base;
> - int num_locks, i, ret;
> + int num_locks, i, ret, base_id;
>
> - if (!pdata)
> + if (!node)
> return -ENODEV;
>
> + ret = of_hwspin_lock_get_base_id(node);
> + if (ret < 0 && ret != -EINVAL)
> + return -ENODEV;
> + base_id = (ret > 0 ? ret : 0);
Does this mean you allow nodes not to have the base_id property? How
do we protect against multiple nodes not having a base_id property
then?
Implicitly assuming a base_id value (zero in this case) may not be always safe.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers
Hi Suman,
On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna wrote:
> +int of_hwspin_lock_get_id(struct device_node *np, int index)
> +{
> + struct hwspinlock_device *bank;
> + struct of_phandle_args args;
> + int id;
> + int ret;
> +
> + ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells",
> index,
> +&args);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&hwspinlock_tree_lock);
> + list_for_each_entry(bank, &hwspinlock_devices, list)
> + if (bank->dev->of_node == args.np)
> + break;
> + mutex_unlock(&hwspinlock_tree_lock);
> + if (&bank->list == &hwspinlock_devices) {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
Is this the validation you mentioned which requires the existence of
"hwspinlock/core: maintain a list of registered hwspinlock banks" ?
I'm not convinced this is needed for several reasons:
- the user isn't using the lock yet at this point, and may only need
the id in order to communicate it to a remote processor
- if the user will try to use the lock prematurely,
hwspin_lock_request_specific should stop her
- moreover, hwspin_lock_request_specific must be the one who validates
the id, since in heterogeneous systems the user may get the id from a
remote processor and not via of_hwspin_lock_get_id
"hwspinlock/core: maintain a list of registered hwspinlock banks"
adds complexity which must be very strongly justified.
If we're not sure there is a strong justification for it, we better
not merge it.
> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
...
> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
Do we really must expose these two helpers globally?
Can we instead make these "static inline" methods and embed them in
hwspinlock_internal.h ?
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 2/5] Documentation: dt: add the omap hwspinlock bindings document
Hi Suman, On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna wrote: > HwSpinlock IP is present only on OMAP4 and other newer SoCs, > which are all device-tree boot only. This patch adds the > DT bindings information for OMAP hwspinlock module. > > Cc: Rob Herring > Signed-off-by: Suman Anna > --- > .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 24 > ++ I need an ACK from a DT maintainer here as well, please try to get it. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 1/5] Documentation: dt: add common bindings for hwspinlock
Hi Suman, On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna wrote: > This patch adds the generic common bindings used to represent > a hwlock device and use/request locks in a device-tree build. > ... > > Cc: Rob Herring > Signed-off-by: Suman Anna > --- > .../devicetree/bindings/hwlock/hwlock.txt | 55 > ++ > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt Please ask a DT maintainer to ACK this - otherwise I can't send this to Linus. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Nov 6, 2014 at 8:24 PM, Suman Anna wrote: > Ping on this. Can you review the latest series v6 [1] and pick it up for > 3.19? The MSM spinlock driver is also blocked/dependent on that series. Sure, it's on my mind, I hope I'll get to it next week. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna wrote: > I am yet to receive any comments on v6, but that series should address > both your need for a probe deferral and Ohad's request to not change any > return types. Please give it a try and let me know if you have any comments. Guys, Just to let you know we have several holidays around here these days, and I intend to review all pending patches immediately soon after. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] remoteproc: add support to handle internal memories
Hi Suman, On Mon, Sep 15, 2014 at 10:39 PM, Suman Anna wrote: > These processors need to use their internal RAM for loading, which is > not for generic usage by the kernel, so defining a CMA block for this > memory doesn't make sense. Ok - so just to make sure I understand, this is physical memory you want to use, which belongs to the remote processor, and which isn't mapped normally by the kernel? > Will it suffice to replace the memcpy() with memcpy_toio()? Yes, memcpy_toio should be fine (and then you don't need to cast the cookie returned by ioremap). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Suman, On Tue, Sep 16, 2014 at 7:14 PM, Suman Anna wrote: > The current remoteproc infrastructure automatically calls rproc_boot > only as part of the rpmsg/virtio stack (in remoteproc_virtio.c), but > since the WkupM3 does not use rpmsg, there is no automatic booting of > the WkupM3 processor. This is the reason why rproc_boot is called as > part of the WkupM3 driver probe sequence. What are your concerns here, > and if you think this is not the right place do invoke rproc_boot, where > do you expect it to be called? The remoteproc layer is meant to hide hardware-specific details, and allow high-level hardware-agnostic code to boot a remote processor, in order to achieve some task, without even caring what kind of hardware it is booting. So generally we have some consumer driver asking remoteproc to boot a remote processor, and in turn, remoteproc asking a hardware-specific vendor driver to take care of the hardware details like actually taking the remote processor out of reset. The consumer driver is some code that deals with some hardware agnostic task. Today the only consumer we have is rpmsg, so it's perfectly reasonable if remoteproc needs to be adapted a bit to accommodate other consumers as they show up. Can you think of any component in your code that is not necessarily hardware specific, and that one day might be useful for other vendors? Can you describe the task you're trying to achieve, the entities involved and the flow between them? > Also do note that, there is no way > at present to retrieve the struct rproc for a given remote processor, to > be able to invoke the rproc_boot from a different layer. It's perfectly ok to make struct rproc public if we have a consumer that requires it. > Splitting this would still require some kind of notifier from remoteproc > for the other layers for them to know that the WkupM3 remote processor > has been loaded and booted successfully. This is also done as part of > the WkupM3 driver at the moment. Are there entities, other than the one that calls rproc_boot, that needs to know that the WkupM3 is up? if so, let's discuss who should notify them - remoteproc or the actual invoker of rproc_boot. It probably depends on who those entities are and what's their relation, if any, to the WkupM3 driver. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Wed, Sep 10, 2014 at 12:10 AM, Kevin Hilman wrote: > What I think you need to do (and what I've recommended at least once in > earlier reviews) put all the (non-rproc) wkup_m3 IPC into into one > driver and create a well-described, well-documented API that the > platform PM code will use. > > IMO, the current "split" is very difficult to read/understand, which > means it will even more difficult to maintain. I strongly agree. A remoteproc driver should generally only register its hardware-specific implementation of the rproc_ops via the remoteproc framework. It is not expected to expose public API of its own - that's why we have the generic remoteproc layer for. It makes perfect sense not to use rpmsg for communications if it's not lightweight enough, but exposing a new set of IPC API should take place in a separate well-documented driver. In addition, the suggested remoteproc driver seems to act both as a low-level hardware-specific driver that plugs into remoteproc (by registering rproc_ops via the remoteproc framework), and also as a high-level driver that utilizes the remoteproc public API (by calling rproc_boot). This alone calls for scrutinizing the design as this is not very intuitive. At least for the remoteproc part: if you could provide a simple and straight-forward remoteproc driver that just implements the rproc_ops, this could be merged very quickly. The rest of the code most probably belongs in a different entity, just like Kevin suggested. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman wrote: > To me, it's not terribly clear how you made the split between this PM > core code an the remoteproc code. In the changelog for the remoteproc > patch, it states it's to "load the firmware for and boot the wkup_m3". > But, while parts of the IPC are here in pm33xx.c, parts of the IPC are > also inside the remoteproc driver, so I'm quite curious if that's OK > with the remoteproc maintainers. Either way, please make it clearer how > and why you made the split, and please isolate the wkup_m3 IPC/protocol > from this code. Think of people wanting to rework/extend the wkup_m3 > firmware. They shouldn't be messing around in here, but rather inside a > driver specificaly for the wkup_m3. I haven't looked at the code very thoroughly yet, but generally a remoteproc driver should only implement the three start/stop/kick rproc_ops, and then register them via the remoteproc framework. Exposing additional API directly from that driver isn't something we immediately want to accept. If relevant, we would generally prefer to extend remoteproc instead, so other platform-specific drivers could utilize that functionality as well. Or rpmsg - if we're missing some IPC functionality. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] remoteproc: add support to handle internal memories
Hi Suman, On Tue, Jul 29, 2014 at 10:33 PM, Suman Anna wrote: > We currently have two usecases. The primary usecase is the WkupM3 > processor on TI Sitara AM335x/AM437x SoCs used for suspend/resume > management. This series is a dependency for the WkupM3 remoteproc driver > that Dave posted [1]. More details are in section 8.1.4.6 of the AM335x > TRM [2]. The program/data sections for this processor all _needs_ to be > in the two internal memory RAMS (16K Unified RAM and 8K Data RAM), and > there is no MMU for this processor. The current RSC_CARVEOUT and > RSC_DEVMEM do not fit to describe this type of memory (we neither > allocate memory through dma api nor do we need to map these into an MMU). Thanks for the details. Can we define a CMA block for these regions, and then just use carveout resource entries instead of the ioremap approach? This may require some changes in remoteproc which we'll need to think about, but it sounds like it may fit the problem better instead of forcing ioremap to provide a regular pointer (we're supposed to use ioremaped memory only with memory primitives such as readl/writel/..). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rpmsg: compute number of buffers to allocate from vrings
Hi Suman, On Tue, Aug 12, 2014 at 7:19 PM, Suman Anna wrote: > Yes, I was playing around with using less buffers in the remoteproc > resource table for the vrings. The remoteproc virtio code creates the > vrings using the number of buffers based on .num field value of struct > fw_rsc_vdev_vring in the resource table. The virtio rpmsg probe code > though tries to set up the receive buffers for the same virtqueue based > on the current hard-coded value of 512 buffers and virtqueue_add_inbuf > would fail as the virtqueue is created with less number of buffers and > throws a WARN_ON. Gotcha - thanks for the details. Limiting the number of buffers in case the vrings are too small makes sense, but let's use RPMSG_NUM_BUFS as an upper bound, so wacky resource tables won't trigger unreasonable memory waste. Something in the lines of: vrp->num_bufs = min(PMSG_NUM_BUFS, virtqueue_get_vring_size(vrp->rvq) * 2); Should probably do the trick. Does this satisfy your requirement? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rpmsg: compute number of buffers to allocate from vrings
Hi Suman, On Thu, Jul 3, 2014 at 11:53 PM, Suman Anna wrote: > The buffers to be used for communication are allocated during > the rpmsg virtio driver's probe, and the number of buffers is > currently hard-coded to 512. Remove this hard-coded value, as > this can vary from one platform to another or between different > remote processors. Instead, rely on the number of buffers the > virtqueue vring is setup with in the first place. Is there a specific problem you bumped into which you are fixing with this approach? Can you please describe it? I'm concerned that coupling the vring size with coherent memory allocated by rpmsg may not be safe. It'd also be an implicit side effect that may surprise users, so let's consider our alternatives. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] hwspinlock for 3.17
The following changes since commit 9a3c4145af32125c5ee39c0272662b47307a8323: Linux 3.16-rc6 (2014-07-20 21:04:16 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock.git tags/hwspinlock-3.17 for you to fetch changes up to ceca89e89ee21a3af050c3ea4c634b3be0e34d51: hwspinlock: enable OMAP build for AM33xx, AM43xx & DRA7xx (2014-07-29 11:46:28 +0300) Two small hwspinlock changes for better OMAP support, coming from Suman Anna. Suman Anna (2): hwspinlock/omap: enable module before reading SYSSTATUS register hwspinlock: enable OMAP build for AM33xx, AM43xx & DRA7xx drivers/hwspinlock/Kconfig | 2 +- drivers/hwspinlock/omap_hwspinlock.c | 27 --- 2 files changed, 21 insertions(+), 8 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: [PATCH 1/2] remoteproc: use a flag to detect the presence of IOMMU
On Mon, Aug 4, 2014 at 6:48 PM, Suman Anna wrote: > On 08/04/2014 06:50 AM, Ohad Ben-Cohen wrote: >> On Tue, Jul 29, 2014 at 7:10 PM, Suman Anna wrote: >>> We are trying to add a remoteproc driver for a small Cortex M3 called >>> the WkupM3 used for suspend/resume management on TI AM335/AM437x SoCs. >>> This processor does not have an MMU. Same is the case with another >>> processor subsystem PRU-ICSS on AM335/AM437x. All these are platform >>> devices, and the current iommu_present check will not scale for the same >>> kernel image to support OMAP4/OMAP5 and AM335/AM437x. >> >> That's perfect, thanks. Can you please add this use case description >> to the commit log? > > I kept the current description generic, but sure, I can add this > specific usecase examples to the commit log. I will post the revised > patches once rc1 is out. Thanks! -- 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] remoteproc: use a flag to detect the presence of IOMMU
On Tue, Jul 29, 2014 at 7:10 PM, Suman Anna wrote: > We are trying to add a remoteproc driver for a small Cortex M3 called > the WkupM3 used for suspend/resume management on TI AM335/AM437x SoCs. > This processor does not have an MMU. Same is the case with another > processor subsystem PRU-ICSS on AM335/AM437x. All these are platform > devices, and the current iommu_present check will not scale for the same > kernel image to support OMAP4/OMAP5 and AM335/AM437x. That's perfect, thanks. Can you please add this use case description to the commit log? This way we'll be able to recover it easily one day if needed. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] remoteproc: add support to handle internal memories
Hi Suman, On Tue, Jul 8, 2014 at 7:22 PM, Suman Anna wrote: > A remote processor may need to load certain firmware sections into > internal memories (eg: RAM at L1 or L2 levels) for performance or > other reasons. Can you please provide as much details as you can about the scenario you need this for? what hardware, what sections, which specific memory, what's the use case, numbers, sizes, everything. I'd like to better understand the use case please. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] remoteproc: use a flag to detect the presence of IOMMU
Hi Suman, On Tue, Jul 8, 2014 at 7:22 PM, Suman Anna wrote: > The remoteproc driver core currently relies on iommu_present() on > the bus the device is on, to perform MMU management. However, this > logic doesn't scale for multi-arch, especially for processors that > do not have an IOMMU. Is there a specific hw/scenario where you need this? Can you please provide more details about it? Ideally we should add them to the commit log as well. > The individual platform implementations are required to set this > flag appropriately. The default setting is to not have an MMU. Let's explicitly set the default please so this would be clear for users reading the code. > Cc: Sjur Brændeland Sjur is no longer with STE, so no point in cc'ing his old email address. > + /* > +* All existing OMAP IPU and DSP processors do have an MMU, and > +* are expected to use MMU, so this statement suffices. > +* XXX: Replace this logic if and when a need arises. The last XXX comment is always true for any kernel code, so I'd drop it. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] hwspinlock/omap: enable module before reading SYSSTATUS register
On Thu, Jul 24, 2014 at 6:13 PM, Suman Anna wrote: >> Is there a specific reason for using the put_sync variant here? If >> not, let's just use the regular put(). > > There was no particular reason, you can change it. Thanks for confirming. I've applied both patches to remoteproc's for-next branch. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] hwspinlock/omap: enable module before reading SYSSTATUS register
Hi Suman, On Thu, Jul 3, 2014 at 2:00 AM, Suman Anna wrote: > The number of hwspinlocks are determined based on the value read > from the IP block's SYSSTATUS register. However, the module may > not be enabled and clocked, and the read may result in a bus error. > > This particular issue is seen rather easily on AM33XX, since the > module wakeup is software controlled, and it is disabled out of > reset. Make sure the module is enabled and clocked before reading > the SYSSTATUS register. > > Signed-off-by: Suman Anna ... > + /* > +* runtime PM will make sure the clock of this module is > +* enabled again iff at least one lock is requested > +*/ > + ret = pm_runtime_put_sync(&pdev->dev); Is there a specific reason for using the put_sync variant here? If not, let's just use the regular put(). Let me know, and I can do the change while applying these two patches. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Suman, On Thu, Jul 3, 2014 at 8:28 PM, Suman Anna wrote: > OK, but we would still require this function to lookup the registered > device from the controller-phandle to retrieve the base_id. Can we retrieve the base_id from the parent DT node itself? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Jul 3, 2014 at 8:35 PM, Suman Anna wrote: > Not at the moment, with the existing platform implementations. So, if I > understand you correctly, you are asking to leave out the xlate ops and > make the of_hwspin_lock_simple_xlate() internal until a need for an > xlate method arises. Yes, please. It seems to me this way we're reducing complexity without compromising anything. > This also means, we only support a value of 1 for #hwlock-cells. When a different #hwlock-cells value shows up, someone would have to write a new xlate implementation anyway. At the same time, the xlate ops could be brought back quite easily. Since we're not imposing anything on the DT data, this doesn't affect our ability to support these scenarios in the future, but unless I'm missing a current use case, these scenarios right now seems somewhat fictional. > But, that would mean DT-based client users would have to invoke two > function calls to request a hwspinlock. We already have an API to get > the lock id given a hwspinlock - hwspin_lock_get_id(), so I added the OF > API for requesting a lock directly rather than giving an OF API for > getting the lock id. This is in keeping in convention with what other > drivers do normally - a regular get and an OF get. I can modify it if > you still prefer the OF API for getting a global lock id, but I feel its > a burden for client users. Let's discuss this. I was actually thinking of the more general use case of an heterogenous system: - driver gets the global lock id from a remote processor - driver then requests the specific lock Looking at these cases, the OF scenario only differs in the small part of fetching the lock id, and if we keep the regular request_specific API we'll have more common code between drivers. What do you think? > Also, do you prefer an open property-name in client users (like I am > doing at the moment) or imposing a standard property name "hwlocks"? Good point - I think we can start with an imposed "hwlocks" name, and evolve in the future as needed (again, only because we're not really imposing anything on the DT data - this is just kernel code that we could always extend as needed). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 05/15] hwspinlock/omap: add support for dt nodes
On Wed, Jul 2, 2014 at 10:42 PM, Suman Anna wrote: > Yeah, I did this since we only had 1 instance, and used the same value > as used in the non-DT legacy code. Once I fold back Patch 8 that adds > the hwlock-base-id property, this will be assigned by reading that property. Sounds good, thanks! -- 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: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna wrote: >> Do we have a use case today that require the xlate() method? >> >> If not, let's remove it as we could always add it back if some new >> hardware shows up that needs it. > > The xlate() method is to support the phandle + args specifier way of > requesting hwlocks, platform implementations are free to implement their > own xlate functions, but the above supports the simplest case of > controller + relative lock index within controller. Do we have a use case for a different implementation other than the simplest case? If not, it seems to me this will just become redundant boilerplate code (every platform will use the simple xlate method). > This function again is to support the phandle + args specifier way of > requesting hwlocks, the hwspin_lock_request_specific() is invoked > internally within this function, so we are still reusing the actual > request code other than handling the DT parsing portion. If we go back > to using global locks in client hwlocks property, we don't need a > of_hwspin_lock_get_id(), the same can be achieved using the existing DT > function, of_property_read_u32_index(). I think you may have misunderstood me, sorry. I'm ok with the phandle + args specifier. I just think we can use it, together with the base_id property, to infer the global lock id from the DT data. This is not only a must to support heterogenous multi-processing systems, it will also substantially simplify the code. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna wrote: >> I'm not sure we need this patch. > > This patch is needed if we use the controller-phandle + args specifier > for requesting hwlocks by a client, as we need to translate > controller-phandle to the corresponding hwspinlock_device. > > Looks like we still don't have a closure on the semantics of how > clients have to request a lock in DT. You are suggesting something like > hwlocks = ; > > whereas this patch is built to support based on comments from > DT-maintainers, > hwlocks = , lock-specifier2>...; I'm actually ok with this suggestion and haven't suggested otherwise. All I propose is that we add the base_id property to the controller node (as you have done in the subsequent patches), and then drivers will be able to infer the global lock id from the DT data by adding the controller's base_id to the lock specifier. Controllers with non standard lock indexing may use an xlate() method if needed but frankly this is fictional right now. We can start without this, and add it later when needed, as this doesn't affect the DT data. With the global lock id in hand, drivers could simply use the existing hwspin_lock_request_specific API to obtain a specific lock, and then we don't need this patch. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 07/15] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna wrote: > HwSpinlocks are supported on AM33xx, AM43xx and DRA7xx SoC > device families as well. The IPs are identical to that of > OMAP4/OMAP5, except for the number of locks. > > Add a depends on to the above family of SoCs to enable the > build support for OMAP hwspinlock driver for any of the above > SoC configs. > > Signed-off-by: Suman Anna This looks too like an independent change. If you want it merged without waiting for the entire series please submit it separately. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 06/15] hwspinlock/omap: enable module before reading SYSSTATUS register
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna wrote: > The number of hwspinlocks are determined based on the value read > from the IP block's SYSSTATUS register. However, the module may > not be enabled and clocked, and the read may result in a bus error. > > This particular issue is seen rather easily on AM33XX, since the > module wakeup is software controlled, and it is disabled out of > reset. Make sure the module is enabled and clocked before reading > the SYSSTATUS register. This seems like a valid fix that is independent of this patch series. Feel free to submit it separately if you want, so we can get it merged. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 05/15] hwspinlock/omap: add support for dt nodes
Hi Suman,
On Thu, May 1, 2014 at 3:34 AM, Suman Anna wrote:
> static int omap_hwspinlock_probe(struct platform_device *pdev)
> {
> - struct hwspinlock_pdata *pdata = pdev->dev.platform_data;
> + struct device_node *node = pdev->dev.of_node;
> struct hwspinlock_device *bank;
> struct hwspinlock *hwlock;
> struct resource *res;
> void __iomem *io_base;
> int num_locks, i, ret;
> + int base_id = 0;
We shouldn't implicitly assume base_id is zero: let's explicitly
protect against potential subsequent invocations of
omap_hwspinlock_probe.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna wrote: > 2. The of_hwspin_lock_simple_xlate() is a simple default >translator function for hwspinlock provider implementations >that use a single cell number for requesting a specific lock >(relatively indexed) within a hwlock bank. Do we have a use case today that require the xlate() method? If not, let's remove it as we could always add it back if some new hardware shows up that needs it. As long as the dt data is unchanged, we should generally only add kernel code that we really need. > 3. The of_hwspin_lock_request_specific() API can be used by >hwspinlock clients to request a specific lock using the >phandle + args specifier. This function relies on the >implementation providing back a relative hwlock id within >the bank from the args specifier. It seems to me we can just introduce a of_hwspin_lock_get_id() method which will provide the global lock id to dt users, with which they could just invoke the existing hwspin_lock_request_specific(). This way we will have more common code between dt users and users who get the lock id from a remote processor. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna wrote: > > The hwspinlock_device structure is used for registering a bank of > locks with the driver core. The structure already contains the > necessary members to identify the bank of locks. The core does not > maintain the hwspinlock_devices itself, but maintains only a radix > tree for all the registered locks. A specific lock can be requested > by users using a global lock id, and any device-specific fields > can be retrieved through a reference to the hwspinlock_device in > each lock. > > The global lock id, however, is not friendly to be requested for > users using the device-tree model. The device-tree representation > will typically have each of the hwspinlock devices represented as > a DT node, and a specific lock can be requested using the device's > phandle and a lock specifier. Add support to the core therefore to > maintain all the registered hwspinlock_devices, so that a device > can be looked up and a specific lock belonging to the device > requested through a phandle + args approach. > > Signed-off-by: Suman Anna I'm not sure we need this patch. It seems to me that the global lock id can be the base_id + lock index, where the former should be a property of the parent dt node, and the latter can just be the phandle argument. Then, with the global lock id in hand, drivers could just use the existing hwspin_lock_request_specific API. If future hardware will bring a more complex scenario, we could then introduce the xlate proposal to resolve it. As long as we're not changing the dt data, and this is all handled by kernel code, then I'd prefer opting for less code now as long as it addresses the requirements. Please let me know if currently there is a use case that can't be addressed using this simpler model. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/7] omap hwspinlock dt support
Hi Suman, On Tue, Mar 18, 2014 at 1:46 AM, Suman Anna wrote: > So far, we have not come across multiple controllers. I see your point, > and I think this also depends on the semantics of how you exchange the > lock id number. The agreement at the moment is on base_ids across > multiple SoC components. If the semantics involve exchanging the > controller instance, for example, then we might get away with it. But > that probably involves adding additional helpers to retrieve controller > instance in addition to lock number, or some other similar functions. Yes, this could be done too, but I agree it is less simple with no real win. > Sorry, I should have rephrased it better - by order, I meant the > inherent order between board early code and other drivers. With DT, we > cannot guarantee that right, as specific locks are requested from drivers. Yeah. > Understood. And we may have to assign the client association with a lock > as well. These are core changes that were actually not needed in the > non-DT case due to the inherent order as stated above. So, are you > suggesting that we add one more property to the controller node to mark > which are reserved, or rely on constructing this through DT tree parsing? I guess this is a question to the DT folks; both approaches work from hwspinlock perspective. In the past Arnd Benoit and myself were happy with adding one more property to the controller node, but this might be somewhat error prone as it leaves room for mistakes - developers can add hwlock phandles and forget to update the reserved property in the controller node. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/7] omap hwspinlock dt support
Hi Suman, On Mon, Mar 17, 2014 at 9:10 PM, Suman Anna wrote: > base_id would be a property (if added) of the hwspinlock controller node, > and from DT perspective, we will be using the phandle for the controller > anyway. So, using a base_id+specifier seems redundant, as the specifier is > already w.r.t a hwspinlock controller node. It is best to leave the base_id > out, just use the specifier. This is pretty much the standard practice > (GPIOs, DMAs, etc all follow this). Do you mean hwspin_lock_get_id() will return just the specifier instead of base_id+specifier? This is problematic, because Linux will not be able to communicate this lock id outside to a different core running a different OS: that OS will have no idea what hwlock controller this is relative to. > Please see the comments from Mark regarding the same on an earlier version. I think I understand and agree with Mark generally, but in this case, the hwlock id is not an implementation detail. Unlike GPIOs/DMAs, this id number is global and system-wide (Linux is just one component in the system, and must use the same predefined id numbers all other cores use, otherwise it will be impossible to use those hwlocks for multi-core synchronization). > Yes, I agree this is an issue if we have to have the base_ids fixed per > controller. Do you see any other way this could work if the base_ids were not predefined? Linux and some other core running a different OS must agree on the id numbers of the hardware locks we have in the system. >> Before DT came along, early board code could have reserved specific >> hwspinlocks if needed. Now with DT, we should add the list of reserved >> locks to the controller node, in order to prevent them from being >> dynamically allocated by others. > > > But that strictly relied on the order of requests without any core changes > in the hwspinlock core, right. I don't think so, you could request a specific lock by id number. > With DT, the early board code is much simplified. Looking at the same > scenario from DT case, it seems kinda redundant to specify a set of reserved > locks both in the controller node, as well as the respective client drivers, > as there is almost no platform data with DT. The only use case for DT client > nodes would be for requesting specific locks. I agree with the problem you > described, and I think it will require a different set of changes to the > core. Exactly. The platform-specific hwspinlock driver will have to inform the core, upon registration, which of the locks are already reserved. In turn, the core will never provide them to clients that dynamically allocate an hwlock: they will be provided only to clients that ask for them specifically (using phandle+specifier). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/7] omap hwspinlock dt support
Hi Suman, On Sat, Mar 15, 2014 at 1:58 AM, Suman Anna wrote: > The series doesn't change the semantics of hwspinlock registration or adds a > new OF controller registration function. Implementations would still need to > register a controller using a base_id and number of locks. The series rather > adds a DT-friendly function _ONLY_ for requesting a specific hwlock, and > there are no restrictions on the args specifier being relative id numbers. > Though this is what the simple default xlate helper does (most common > usage), the added xlate ops and #hwlock-cells should allow individual > implementation drivers to adjust any variations, and return a relative lock > w.r.t its registered base_id, as this is how a lock gets registered in the > first place. I might be missing something, but why can't we have the specifier+base_id be the hwlock id and then we can entirely drop most of the core changes in this patch-set? I realize we couldn't easily support sparse id numbers, but not sure this is relevant to hwspinlocks? do we have a use case that couldn't be supported in this case? > I actually started out this series with the base_id property, and dropped it > in v3 based on comments looking at it from the request-specific-lock > semantics with DT. That said, the drivers still need to manage a 'base_id' > needed for registration when they get probed for multiple controllers. > Getting the base_id from DT _may_ be useful just for registration purposes, > but for requesting a hwlock, a controller phandle and an implementation > defined args-specifier should suffice IMHO. How could drivers know what the base_id is if DT doesn't provide it? please note that we can't depend on order of controller probing; the hwlock id numbers cannot depend on implementation details. > The exact notion of informing the hwspinlock core about a list of reserved > locks is missing at the moment (even in the non-DT case). I am not sure if > this got lost during the conversion of the registration from per lock to > registering a bank of locks together, or if it is implied by the base_id + > num_locks combination. The core today supports requesting only those locks > that were actually registered, whether allocating a free one dynamically or > giving a specific one. Before DT came along, early board code could have reserved specific hwspinlocks if needed. Now with DT, we should add the list of reserved locks to the controller node, in order to prevent them from being dynamically allocated by others. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Fri, Mar 14, 2014 at 5:23 PM, Josh Cartwright wrote: > So, are you suggesting that because fatal errors should be "extremely > rare", a consuming driver should just assume that if NULL is returned > from a hwspin_lock_request*() function that it was the "device not yet > probed" case that was hit? No - it's not the scarcity, it's the severity. The error path that will be optimized here is an invalid id. If this happens, the consumer will crash and burn, and I'm not sure that slightly optimizing his death is very interesting? BTW the hwspinlock core once did use ERR_PTR and friends, and it was changed due to convincing arguments against that methodology on this mailing list. We can change it back but we need a strong(er) case. > Note that having the consumer/hwspinlock device relationship modeled in > devicetree introduces more potential failure cases... Yeah. Even the error above, presumed to be EPROBE_DEFER, may be a symptom of some other fatal error that occurred, and we can't be sure that a future request will surely be satisfied. So before we bloat our code, I suggest that we wait for consumers to show up and see if there's real benefit. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 0/7] omap hwspinlock dt support
Hi Suman, Mark, On Mon, Feb 24, 2014 at 8:14 PM, Suman Anna wrote: > Mark, Ohad, ... > Gentle reminder, can you provide your acks/comments? Sorry for the late jump in. I have a few comments: - Hardware spinlocks must have global and system-wide id numbers; these numbers cannot be maintained internally by the Linux Kernel. Think of an SoC with several asynchronous heterogeneous processors, each of which is running a different OS, and they all need to use a specific hardware spinlock in order to share some resource. For that to happen, every hwlock must have a predefined and deterministic id number which is global in the system. We can't have those id numbers be relative to an hwlock "controller", and we can't have two hwlock "controllers" share the same id numbers. - I suspect the simplest and most straight forward way to achieve this is by (a) bringing back the concept of the base_id property, and (b) letting the global hwlock id be the DT identifier (plus the base_id) and then providing it directly to the drivers when needed. The latter is required in order to support dynamically allocation of hwlocks, in which case Linux must know the global system-wide id number, and then share it with the other asynchronous OSes via some IPC. - If we feel there's no way any system is going to have more than a single hwlock controller, then we can live without a base_id property, but then this needs to be clearly documented and prohibited. Today both the hwlock DT representation, and the coupled kernel code, implicitly allow this anomaly to exist. - Hwlock controller nodes should have a list of reserved locks (those locks for which other nodes have a phandle+identifier pointing at) to prevent those locks from being dynamically allocated by eager drivers. Most of these issues were discussed Arnd, Benoit and myself back then, please see below: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/064265.html Let's discuss, Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson wrote: > When introducing the ability to reference a hwspin lock via a phandle > in device tree it makes a big difference to be able to differ between > the case of "initialization failed" or "device not yet probed"; so > that the client knows if it should fail or retry later. I'm not convinced. The only advantage this brings is to avoid retrying in case a fatal error has occurred. Such fatal errors are extremely rare, and when they show up - extremely painful, and I suspect that optimizing them wouldn't be a big win. OTOH, keeping the code easier to read and less error prone is a big win. I prefer we keep it simple for now. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Hi Suman, On Tue, Mar 4, 2014 at 7:38 PM, Suman Anna wrote: Do you have any objections to the return code convention change? >>> >>> Unless strictly needed, I prefer we don't switch to the ERR_PTR code >>> convention, as it reduces code readability and increases chances of >>> user bugs. >>> >>> In our case, switching to ERR_PTR and friends seems only to optimize a >>> few error paths, and I'm not sure it's a big win over simplicity. >> >> >> When introducing the ability to reference a hwspin lock via a phandle >> in device tree it makes a big difference to be able to differ between >> the case of "initialization failed" or "device not yet probed"; so >> that the client knows if it should fail or retry later. >> > > Can you confirm the changes you want me to make, so that I can refresh and > post a v5 for 3.15? Sorry, I missed your replies for some reason. I prefer we stick with the current error handling code because I find the alternative inferior (as long as it's not strictly needed). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna wrote: > On 02/07/2014 04:49 PM, Bjorn Andersson wrote: >> It seems to be standard practice to pass the error value back to the >> consumer, so you should >> return ERR_PTR(ret); here instead of the NULL... > > > I have modelled the return values in this function based on the return > values in the existing hwspin_lock_request interfaces. I would need to > change those functions as well. > > Ohad, > Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] remoteproc fixes
With the lists this time On Wed, Jul 10, 2013 at 6:17 PM, Ohad Ben-Cohen wrote: > The following changes since commit 9e895ace5d82df8929b16f58e9f515f6d54ab82d: > > Linux 3.10-rc7 (2013-06-22 09:47:31 -1000) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git > tags/remoteproc-3.11-fixes > > for you to fetch changes up to 95cee62cb4776a65229a6b6d5969be56589d95c1: > > remoteproc: Cocci spatch "memdup.spatch" (2013-07-01 17:23:58 +0300) > > > Trivial remoteproc fixes by Suman Anna, Wei Yongjun and Thomas Meyer. > > > Suman Anna (3): > remoteproc: fix checkpatch errors in remoteproc code > remoteproc/omap: fix a sparse warning > remoteproc: free carveout memories only after unmapping them > > Thomas Meyer (1): > remoteproc: Cocci spatch "memdup.spatch" > > Wei Yongjun (1): > remoteproc: fix error return code in rproc_fw_boot() > > drivers/remoteproc/remoteproc_core.c | 24 > drivers/remoteproc/remoteproc_debugfs.c | 3 +-- > drivers/remoteproc/remoteproc_internal.h | 4 ++-- > include/linux/platform_data/remoteproc-omap.h | 2 +- > 4 files changed, 16 insertions(+), 17 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: find real users and drivers of rpmsg
Hi, On Mon, Jul 8, 2013 at 10:31 AM, Barry Song <21cn...@gmail.com> wrote: > hi Ohad/all, > i am trying to find some real users of rpmsg, here i only get > samples/rpmsg/rpmsg_client_sample.c, does it mean other real drivers > are out of mainline? Yes > where could i get them? TI maintains them in internal trees, some of which might be public. I'm looping in Suman from TI who might be able to refer you to some > i am also trying to find source codes running in Cortex-M3 which use > rpmsg and hope to find some Linux userspace codes which use rpmsg too. > Thanks > barry -- 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
[GIT PULL] remoteproc for 3.10
There's a trivial merge conflict with a Kconfig typo that got fixed during the rc cycle, but otherwise: The following changes since commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9: Linux 3.9-rc2 (2013-03-10 16:54:19 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git tags/remoteproc-3.10 for you to fetch changes up to b9777859ec015a78dae1476e317d04f851bfdd0d: remoteproc: fix kconfig dependencies for VIRTIO (2013-04-21 16:30:22 +0300) This pull request contains: - Some refactoring, cleanups and small improvements from Sjur Brændeland. The improvements are mainly about better supporting varios virtio properties (such as virtio's config space, status and features). I now see that I messed up while commiting one of Sjur's patches and erroneously put myself as the author, as well as letting a nasty typo sneak in. I will not fix this in order to avoid rebasing the patches. Sjur - sorry! - A new remoteproc driver for OMAP-L13x (technically a DaVinci platform) from Robert Tivy. - Extend OMAP support to OMAP5 as well, from Vincent Stehlé. - Fix Kconfig VIRTUALIZATION dependency, from Suman Anna (a non-critical fix which arrived late during the rc cycle). -------- Ohad Ben-Cohen (1): remoteproc: perserve resource table data Robert Tivy (2): remoteproc: support default firmware name in rproc_alloc() remoteproc/davinci: add a remoteproc driver for OMAP-L13x DSP Sjur Brændeland (7): remoteproc: refactor rproc_elf_find_rsc_table() remoteproc: add find_loaded_rsc_table firmware ops remoteproc: parse STE-firmware and find resource table address remoteproc: code cleanup of resource parsing remoteproc: calculate max_notifyid by counting vrings remoteproc: support virtio config space. remoteproc: set vring addresses in resource table Suman Anna (1): remoteproc: fix kconfig dependencies for VIRTIO Vincent Stehlé (1): remoteproc/omap: support OMAP5 too drivers/remoteproc/Kconfig | 27 ++- drivers/remoteproc/Makefile| 1 + drivers/remoteproc/da8xx_remoteproc.c | 324 + drivers/remoteproc/remoteproc_core.c | 211 --- drivers/remoteproc/remoteproc_elf_loader.c | 100 ++--- drivers/remoteproc/remoteproc_internal.h | 15 +- drivers/remoteproc/remoteproc_virtio.c | 79 +-- drivers/remoteproc/ste_modem_rproc.c | 45 ++-- include/linux/remoteproc.h | 13 +- 9 files changed, 677 insertions(+), 138 deletions(-) create mode 100644 drivers/remoteproc/da8xx_remoteproc.c -- 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
[GIT PULL] rpmsg for 3.10
The following changes since commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9: Linux 3.9-rc2 (2013-03-10 16:54:19 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/rpmsg.git tags/rpmsg-3.10 for you to fetch changes up to 397944df3290ddc46dcc6a08cd71fb560700431b: rpmsg: fix kconfig dependencies for VIRTIO (2013-04-21 16:32:29 +0300) A small pull request consisting of: - Make rpmsg process all pending messages instead of just one, from Robert Tivy - Fix Kconfig dependency on VIRTUALIZATION, from Suman. Note: this was submitted late during the 3.9 rc cycle and it seemed appropriate to wait with it for the merge window. - Belated addition of an rpmsg entry to the MAINTAINERS file. People seem to look for this. Ohad Ben-Cohen (1): MAINTAINERS: add rpmsg entry Robert Tivy (1): rpmsg: process _all_ pending messages in rpmsg_recv_done Suman Anna (1): rpmsg: fix kconfig dependencies for VIRTIO MAINTAINERS | 8 +++ drivers/rpmsg/Kconfig| 1 + drivers/rpmsg/virtio_rpmsg_bus.c | 49 3 files changed, 44 insertions(+), 14 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
[GIT PULL] hwspinlock for 3.10
The following changes since commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9: Linux 3.9-rc2 (2013-03-10 16:54:19 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock.git tags/hwspinlock-3.10 for you to fetch changes up to 8ae053d62e88c400330ebaf27558bf02dde5a1fa: hwspinlock/omap: support OMAP5 as well (2013-04-05 09:11:17 +0300) A single patch from Vincent extending OMAP's hwspinlock support to OMAP5. Vincent Stehlé (1): hwspinlock/omap: support OMAP5 as well drivers/hwspinlock/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 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: adding rpmsg.git to linux next
Hi Stephen, On Tue, Apr 16, 2013 at 3:07 AM, Stephen Rothwell wrote: > On Mon, 15 Apr 2013 09:28:17 +0300 Ohad Ben-Cohen wrote: >> Could you please add: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/ohad/rpmsg.git#for-next >> >> to linux-next to include new stuff coming from Rob? > > Well, you could tell me something about it. Like what is in it and how > it will be merged up to Linus. Does this have anything to do with the > remoteproc tree I already include? Who is the official maintainer (there > is no mention of drivers/rpmsg in the MAINTAINERS file). Sorry for not mentioning this. This tree isn't new and is being used to send pull requests to Linus for some time. I guess that most (all?) of the pull requests contained fixes which needed no linux-next presence, so it never occurred to me the tree isn't part of linux-next. Some background: Rpmsg is a virtio-based messaging bus that allows kernel drivers to communicate with entities running on remote processors available on the system. In turn, drivers could then expose appropriate user space interfaces, if needed. Rpmsg is essentially a multiplexer providing communication channels which rpmsg drivers utilizes; it does not handle the physical state of the remote processor, and in fact is completely decoupled from remoteproc, which does focus on the physical state of the remote processor (but has no communications aspect). For much more information, please see Documentation/rpmsg.txt. I maintain rpmsg (by sending pull requests to Linus), and for some reason never added a MAINTAINERS entry for it. I can fix that quite easily, let me know if this helps: commit d8115db52b99eefb99bcbf992edc349e691b4ca7 Author: Ohad Ben-Cohen Date: Tue Apr 16 20:34:49 2013 +0300 MAINTAINERS: add rpmsg entry People and scripts look for this. Signed-off-by: Ohad Ben-Cohen diff --git a/MAINTAINERS b/MAINTAINERS index 9561658..eaca6c8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6604,6 +6604,14 @@ F: drivers/remoteproc/ F: Documentation/remoteproc.txt F: include/linux/remoteproc.h +REMOTE PROCESSOR MESSAGING (RPMSG) SUBSYSTEM +M: Ohad Ben-Cohen +T: git git://git.kernel.org/pub/scm/linux/kernel/git/ohad/rpmsg.git +S: Maintained +F: drivers/rpmsg/ +F: Documentation/rpmsg.txt +F: include/linux/rpmsg.h + RFKILL M: Johannes Berg L: linux-wirel...@vger.kernel.org Hope this all helps, please let me know if anything else is needed. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
adding rpmsg.git to linux next
Hi Stephen, Could you please add: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/rpmsg.git#for-next to linux-next to include new stuff coming from Rob? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] an hwspinlock fix for 3.9
The following changes since commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9: Linux 3.9-rc2 (2013-03-10 16:54:19 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock.git tags/hwspinlock-3.9-fix for you to fetch changes up to c10b90d85a5126d25c89cbaa50dc9fdd1c4d001a: hwspinlock: fix __hwspin_lock_request error path (2013-04-05 17:45:11 +0300) An hwspinlock fix from Li Fei, taking care of a faulty error path. Li Fei (1): hwspinlock: fix __hwspin_lock_request error path drivers/hwspinlock/hwspinlock_core.c | 2 ++ 1 file changed, 2 insertions(+) -- 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
[GIT PULL] remoteproc fixes for 3.9
The following changes since commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9: Linux 3.9-rc2 (2013-03-10 16:54:19 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git tags/remoteproc-3.9-fixes for you to fetch changes up to c7426bce5933d16b492a34e42ae77e26fceddff6: remoteproc: fix FW_CONFIG typo (2013-04-07 15:11:27 +0300) 4 small remoteproc fixes: - Suman fixed an issue that crawled in with the move to the new idr_alloc interface in 3.9 - Dmitry fixed an STE specific memory leak - Sjur fixed an error path in the core - Rob fixed a Kconfig typo Dmitry Tarnyagin (1): remoteproc/ste: fix memory leak on shutdown Robert Tivy (1): remoteproc: fix FW_CONFIG typo Sjur Brændeland (1): remoteproc: fix error path of handle_vdev Suman Anna (1): remoteproc: fix the error check for idr_alloc drivers/remoteproc/Kconfig | 2 +- drivers/remoteproc/remoteproc_core.c | 6 -- drivers/remoteproc/ste_modem_rproc.c | 7 ++- 3 files changed, 11 insertions(+), 4 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: [PATCH] hwspinlock: depend on OMAP4 or OMAP5
On Wed, Feb 27, 2013 at 7:10 PM, Vincent Stehlé wrote: > OMAP5 has spinlocks, too. > > Signed-off-by: Vincent Stehlé > Cc: Ohad Ben-Cohen > Cc: Tony Lindgren Thanks, applied to hwspinlock-next. -- 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] remoteproc/omap: depend on omap4 or 5
On Mon, Feb 18, 2013 at 1:06 PM, Vincent Stehlé wrote: > Signed-off-by: Vincent Stehlé Thanks, applied to remoteproc-next. -- 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/9] drivers: mailbox: framework creation
Hi Omar, On Fri, Dec 21, 2012 at 9:33 PM, Omar Ramirez Luna wrote: > Yes, I made the changes, for tidspbridge and remoteproc, I will submit > both for review, based on this series. Great, thanks. Please note that when we do eventually merge this, we need your updates to be squashed into Loic's patches so we don't break bisectibility. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] omap: iommu: fixing NULL pointer return value
Hi Guillaume, You should also cc Joerg and the iommu ml for iommu patches (cc'ing now). On Fri, Dec 21, 2012 at 3:36 PM, Guillaume Aubertin wrote: > the returned NULL pointer is not detected by IS_ERR(), and then > de-referenced in the calling function, omap_iommu_attach_dev(). > > Signed-off-by: Guillaume Aubertin Acked-by: Ohad Ben-Cohen > --- > drivers/iommu/omap-iommu.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c > index badc17c..a0fcad2 100644 > --- a/drivers/iommu/omap-iommu.c > +++ b/drivers/iommu/omap-iommu.c > @@ -861,7 +861,7 @@ static struct omap_iommu *omap_iommu_attach(const char > *name, u32 *iopgd) > (void *)name, > device_match_by_alias); > if (!dev) > - return NULL; > + return ERR_PTR(-ENODEV); > > obj = to_iommu(dev); > > -- > 1.7.0.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] drivers: mailbox: framework creation
On Thu, Dec 20, 2012 at 9:19 PM, Olof Johansson wrote: > While we can make the branch stable, would it make sense to make > remoteproc for omap depend on !multiplatform during the transition, to > reduce dependencies a little? Either way works, but it'd be nice to > keep them independent if we can. I'm not sure multiplatform is the culprit; OMAP's remoteproc driver heavily depends on this mailbox code, and obviously breaks with this patch-set if only for the the naming changes. We'll need this patch set to update omap's remoteproc as well so at least we don't break bisectibility, though running a sanity test before merging would be even nicer (Loic I can help if you don't have a panda board). BTW - grep shows that tidspbridge is using the mailbox code too, but it's in staging and I'm not sure it gets much love. Nevertheless, as long as it's there we should at least update it with the new API as well. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] mailbox: rename omap_mbox in mailbox
Hi Loic, On Tue, Dec 18, 2012 at 3:10 PM, Loic Pallardy wrote: > In order to create a generic mailbox framework, functions > and structures should be renamed in mailbox. This patch should also update omap_remoteproc.c so the build doesn't break (and if you have a panda board around it might be nice to run the remoteproc sample to make sure things still generally work). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] remoteproc: a single fix for 3.7
Hi Linus, The following changes since commit 6f0c0580b70c89094b3422ba81118c7b959c7556: Linux 3.7-rc2 (2012-10-20 12:11:32 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git tags/rproc-3.7-fix for you to fetch changes up to dab55bbafdb491ce2c3f2d7136e54101e948b911: remoteproc: fix error path of ->find_vqs (2012-11-29 10:05:09 +0200) A single remoteproc fix for an error path issue reported by Ido Yariv. ---- Ohad Ben-Cohen (1): remoteproc: fix error path of ->find_vqs drivers/remoteproc/remoteproc_virtio.c | 18 -- 1 file changed, 12 insertions(+), 6 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: [PATCH v5 0/5] OMAP: iommu: hwmod, reset handling and runtime PM
On Tue, Nov 20, 2012 at 3:05 AM, Omar Ramirez Luna wrote: > These patches are needed for remoteproc to work on OMAP4. > > Introduced iommu hwmod support for OMAP3 (iva, isp) and > OMAP4 (ipu, dsp), along with the corresponding runtime PM > and routines to deassert reset lines, enable/disable clocks > and configure sysc registers. I tested the entire series with remoteproc/rpmsg on OMAP4 and it looks good. Since this is super needed, and have already been going on for quite a while, I'd really like to see this go in. We could then incrementally deal with any outstanding comment or issue that might show up. Joerg, can you please take it (or at least its first four patches) ? We have Tony's ack for the mach-omap2 parts. Tested-by: Ohad Ben-Cohen Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
Hi Omar, On Thu, Nov 15, 2012 at 6:53 PM, Omar Ramirez Luna wrote: > On 14 November 2012 03:54, Ohad Ben-Cohen wrote: >> Most of the above questions imply this patch not only converts the >> iommu to runtime PM, but may carry additional changes that may imply >> previous implementation is sub-optimal. I hope we can clearly document >> the motivation behind these changes too (maybe even consider >> extracting them to a different patch ?). > > I didn't want to extract this changes into different patches since > they could be included in this one, otherwise it would look like lines > adding and then deleting runtime pm functions. > > I do agree description is missing, again I thought I had done this > somewhere but looks like I didn't, will update these. If you think > these should be different patches please let me know, otherwise I > would like to keep a single *documented* patch. It seems like there are 3 different logical changes here: 1. remove clk_* invocations from iommu_fault_handler() 2. keep clocks enabled as long as iommu is enabled 3. convert to runtime pm If we split this to three patches in this order, you won't have to add and remove runtime pm functions. Let's do it, please. It's a small nuisance now, but may be really helpful in the future when someone tries to debug stuff and understand the motivation behind these changes. It'd make it much easier for you to document the changes, too: you have an entire commit log per change. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] remoteproc: fix error path of ->find_vqs
Eliminate an erroneous invocation of rproc_shutdown inside
the error path of rproc_virtio_find_vqs.
Reported-by: Ido Yariv
Signed-off-by: Ohad Ben-Cohen
---
drivers/remoteproc/remoteproc_virtio.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index e7a4780..9e198e5 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -120,15 +120,11 @@ static struct virtqueue *rp_find_vq(struct virtio_device
*vdev,
return vq;
}
-static void rproc_virtio_del_vqs(struct virtio_device *vdev)
+static void __rproc_virtio_del_vqs(struct virtio_device *vdev)
{
struct virtqueue *vq, *n;
- struct rproc *rproc = vdev_to_rproc(vdev);
struct rproc_vring *rvring;
- /* power down the remote processor before deleting vqs */
- rproc_shutdown(rproc);
-
list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
rvring = vq->priv;
rvring->vq = NULL;
@@ -137,6 +133,16 @@ static void rproc_virtio_del_vqs(struct virtio_device
*vdev)
}
}
+static void rproc_virtio_del_vqs(struct virtio_device *vdev)
+{
+ struct rproc *rproc = vdev_to_rproc(vdev);
+
+ /* power down the remote processor before deleting vqs */
+ rproc_shutdown(rproc);
+
+ __rproc_virtio_del_vqs(vdev);
+}
+
static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
@@ -163,7 +169,7 @@ static int rproc_virtio_find_vqs(struct virtio_device
*vdev, unsigned nvqs,
return 0;
error:
- rproc_virtio_del_vqs(vdev);
+ __rproc_virtio_del_vqs(vdev);
return ret;
}
--
1.7.10.rc3.1067.gb129051
--
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 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm
Hi Omar, On Wed, Nov 14, 2012 at 4:34 AM, Omar Ramirez Luna wrote: > Use runtime PM functionality interfaced with hwmod enable/idle > functions, to replace direct clock operations and sysconfig > handling. > > Dues to reset sequence, pm_runtime_put_sync must be used, to avoid > possible operations with the module under reset. There are some changes here that might not be trivial to understand in hindsight; any chance you can add more explanations (even only in the commit log) regarding: > @@ -160,11 +160,10 @@ static int iommu_enable(struct omap_iommu *obj) ... > - clk_enable(obj->clk); > + pm_runtime_get_sync(obj->dev); > > err = arch_iommu->enable(obj); > > - clk_disable(obj->clk); > return err; > } Why do we remove clk_disable here (instead of replacing it with a _put variant) ? > @@ -306,7 +303,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, > struct iotlb_entry *e) > if (!obj || !obj->nr_tlb_entries || !e) > return -EINVAL; > > - clk_enable(obj->clk); > + pm_runtime_get_sync(obj->dev); If iommu_enable no longer disables obj->clk before returning, do we really need to call ->get here (and in all the other similar instances) ? > @@ -816,9 +813,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 do we remove the clk_ invocations here (instead of replacing them with get/put variants) ? Most of the above questions imply this patch not only converts the iommu to runtime PM, but may carry additional changes that may imply previous implementation is sub-optimal. I hope we can clearly document the motivation behind these changes too (maybe even consider extracting them to a different patch ?). > @@ -990,6 +981,9 @@ static int __devinit omap_iommu_probe(struct > platform_device *pdev) > goto err_irq; > platform_set_drvdata(pdev, obj); > > + pm_runtime_irq_safe(obj->dev); Let's also document why _irq_safe is needed here ? Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/6] Move rest of omap-iommu to live in drivers/iommu
On Fri, Nov 2, 2012 at 9:23 PM, Tony Lindgren wrote: > We need to move the iommu code to live under drivers > for arm common zImage support. For the iommu changes in the entire series: Acked-by: Ohad Ben-Cohen Joerg, it might relieve some pain if this will go through Tony's tree, as there are some OMAP platform iommu changes coming in from Omar as well (part of which might have already been merged in the omap branches). Hope it's ok with both of you guys? Omar, do you still have any iommu changes coming in ? Can we get everything merged to the same tree ? Thanks! Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ARM: OMAP2+: Move plat/iovmm.h to include/linux/omap-iommu.h
Hi Laurent, On Fri, Oct 26, 2012 at 11:35 AM, Laurent Pinchart wrote: > That's on my to-do list, as well as porting the OMAP3 ISP driver to videobuf2, > adding DT support, moving to the common clock framework (when that will be > available for the OMAP3), supporting missing V4L2 features, ... All this in my > spare time of course, otherwise it wouldn't be fun, would it ? ;-) Hmm, seems like a short to-do list ;) > I would also like to move the tidspbridge to the DMA API, but I think we'll > need to move step by step there, and using the OMAP IOMMU and IOVMM APIs as an > intermediate step would allow splitting patches in reviewable chunks. I know > it's a step backwards in term of OMAP IOMMU usage, but that's in my opinion a > temporary nuisance to make the leap easier. Since tidspbridge is in staging I guess it's not a problem, though it sounds to me like using the correct API in the first place is going to make less churn. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ARM: OMAP2+: Move plat/iovmm.h to include/linux/omap-iommu.h
On Thu, Oct 25, 2012 at 11:39 PM, Tony Lindgren wrote: >> > Joerg and Ohad, do you have any opinions on this? I agree that there's some merit in having a separate header file for IOVMM, since it's a different layer from the IOMMU API. But in reality it's tightly coupled with OMAP's IOMMU, and ideally it really should go away and be replaced with the DMA API. For this reason, and for the fact that anyway there's only a single user for it (omap3isp) and there will never be any more, maybe we shouldn't pollute include/linux anymore. Anyone volunteering to remove IOVMM and adapt omap3isp to the DMA API instead ? ;) Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)
On Wed, Oct 24, 2012 at 3:54 AM, Tony Lindgren wrote:
> I don't think mach-davinci has wl12xx_board_init() available?
> Maybe leave this out or define also also somewhere for mach-davinci?
Yeah let's leave davinci out for now:
>From 665bcaa7ef0ed385cf1765f2d4503bf84aaf488a Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen
Date: Sun, 14 Oct 2012 20:16:01 +0200
Subject: [PATCH] ARM: OMAP: don't print any error when wl12xx isn't
configured
Stop intimidating users with scary wlan error messages in case wl12xx
support wasn't even built.
In addition, when wl12xx_set_platform_data() fails, don't bother
registering wl12xx's fixed regulator device (on the relevant
boards).
While we're at it, extract the wlan init code to a dedicated function to
make (the relevant boards') init code look a bit nicer.
Compile tested only.
Reported-by: Russell King
Signed-off-by: Ohad Ben-Cohen
---
arch/arm/mach-omap2/board-4430sdp.c | 7 +++---
arch/arm/mach-omap2/board-omap3evm.c | 6 ++---
arch/arm/mach-omap2/board-omap3pandora.c | 9 +++-
arch/arm/mach-omap2/board-omap4panda.c | 20 -
arch/arm/mach-omap2/board-zoom-peripherals.c | 15 -
arch/arm/mach-omap2/common-board-devices.c | 33
arch/arm/mach-omap2/common-board-devices.h | 2 ++
7 files changed, 69 insertions(+), 23 deletions(-)
diff --git a/arch/arm/mach-omap2/board-4430sdp.c
b/arch/arm/mach-omap2/board-4430sdp.c
index 3669c12..bc48679 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -825,10 +825,11 @@ static void __init omap4_sdp4430_wifi_init(void)
int ret;
omap4_sdp4430_wifi_mux_init();
- omap4_sdp4430_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
- ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
+
+ ret = wl12xx_board_init(&omap4_sdp4430_wlan_data, GPIO_WIFI_IRQ);
if (ret)
- pr_err("Error setting wl12xx data: %d\n", ret);
+ return;
+
ret = platform_device_register(&omap_vwlan_device);
if (ret)
pr_err("Error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3evm.c
b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..8e3a075 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -636,10 +636,10 @@ static void __init omap3_evm_wl12xx_init(void)
int ret;
/* WL12xx WLAN Init */
- omap3evm_wlan_data.irq = gpio_to_irq(OMAP3EVM_WLAN_IRQ_GPIO);
- ret = wl12xx_set_platform_data(&omap3evm_wlan_data);
+ ret = wl12xx_board_init(&omap3evm_wlan_data, OMAP3EVM_WLAN_IRQ_GPIO);
if (ret)
- pr_err("error setting wl12xx data: %d\n", ret);
+ return;
+
ret = platform_device_register(&omap3evm_wlan_regulator);
if (ret)
pr_err("error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3pandora.c
b/arch/arm/mach-omap2/board-omap3pandora.c
index 00a1f4a..bfdc7aa 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -543,13 +543,10 @@ static void __init pandora_wl1251_init(void)
if (ret < 0)
goto fail;
- pandora_wl1251_pdata.irq = gpio_to_irq(PANDORA_WIFI_IRQ_GPIO);
- if (pandora_wl1251_pdata.irq < 0)
- goto fail_irq;
-
pandora_wl1251_pdata.use_eeprom = true;
- ret = wl12xx_set_platform_data(&pandora_wl1251_pdata);
- if (ret < 0)
+
+ ret = wl12xx_board_init(&pandora_wl1251_pdata, PANDORA_WIFI_IRQ_GPIO);
+ if (ret)
goto fail_irq;
return;
diff --git a/arch/arm/mach-omap2/board-omap4panda.c
b/arch/arm/mach-omap2/board-omap4panda.c
index bfcd397..f203cd9 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -486,24 +486,32 @@ static void omap4_panda_init_rev(void)
}
}
+static void __init omap4_panda_wlan_init(void)
+{
+ int ret;
+
+ ret = wl12xx_board_init(&omap_panda_wlan_data, GPIO_WIFI_IRQ);
+ if (ret)
+ return;
+
+ ret = platform_device_register(&omap_vwlan_device);
+ if (ret)
+ pr_err("error registering wl12xx's fixed regulator: %d\n", ret);
+}
+
static void __init omap4_panda_init(void)
{
int package = OMAP_PACKAGE_CBS;
- int ret;
if (omap_rev() == OMAP4430_REV_ES1_0)
package = OMAP_PACKAGE_CBL;
omap4_mux_init(board_mux, NULL, package);
- omap_panda_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
- ret = wl12xx_set_platform_data(&omap_panda_wlan_data);
- if (ret)
- pr_err("error setting wl12xx data: %d\n",
Re: Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)
On Tue, Oct 23, 2012 at 9:37 AM, Igor Grinberg wrote: >> + ret = wl12xx_set_platform_data(wlan_data); >> + /* bail out silently in case wl12xx isn't configured */ >> + if (ret == -ENOSYS) >> + return ret; > > Since we have the function ifdef'ed, I don't think we need > the ENOSYS check, do we? If we want to be strict, we better not remove it. It's an interface that hides the internal implementation, and it's just better not to assume anything beyond the return values and their meanings. This way if WLAN folks change something in the future, we don't need to update all the boards code again. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)
On Fri, Oct 19, 2012 at 7:07 PM, Tony Lindgren wrote:
...
> We could optimize away a minimal amount of code for many configurations
> with the ifdef? :)
Sure, here goes (compile tested only):
>From 6b82365da2c04986e647d06c285197efece7cb34 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen
Date: Sun, 14 Oct 2012 20:16:01 +0200
Subject: [PATCH] ARM: OMAP: don't print any error when wl12xx isn't
configured
Stop intimidating users with scary wlan error messages in case wl12xx
support wasn't even built.
In addition, when wl12xx_set_platform_data() fails, don't bother
registering wl12xx's fixed regulator device (on the relevant
boards).
While we're at it, extract the wlan init code to a dedicated function to
make (the relevant boards') init code look a bit nicer.
Compile tested only.
Reported-by: Russell King
Signed-off-by: Ohad Ben-Cohen
---
arch/arm/mach-davinci/board-da850-evm.c | 8 ++-
arch/arm/mach-omap2/board-4430sdp.c | 7 +++---
arch/arm/mach-omap2/board-omap3evm.c | 6 ++---
arch/arm/mach-omap2/board-omap3pandora.c | 9 +++-
arch/arm/mach-omap2/board-omap4panda.c | 20 -
arch/arm/mach-omap2/board-zoom-peripherals.c | 15 -
arch/arm/mach-omap2/common-board-devices.c | 33
arch/arm/mach-omap2/common-board-devices.h | 2 ++
8 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da850-evm.c
b/arch/arm/mach-davinci/board-da850-evm.c
index 32ee3f8..7243c22 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1401,13 +1401,9 @@ static __init int da850_wl12xx_init(void)
goto free_wlan_en;
}
- da850_wl12xx_wlan_data.irq = gpio_to_irq(DA850_WLAN_IRQ);
-
- ret = wl12xx_set_platform_data(&da850_wl12xx_wlan_data);
- if (ret) {
- pr_err("Could not set wl12xx data: %d\n", ret);
+ ret = wl12xx_board_init(&da850_wl12xx_wlan_data, DA850_WLAN_IRQ);
+ if (ret)
goto free_wlan_irq;
- }
return 0;
diff --git a/arch/arm/mach-omap2/board-4430sdp.c
b/arch/arm/mach-omap2/board-4430sdp.c
index 3669c12..bc48679 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -825,10 +825,11 @@ static void __init omap4_sdp4430_wifi_init(void)
int ret;
omap4_sdp4430_wifi_mux_init();
- omap4_sdp4430_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
- ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
+
+ ret = wl12xx_board_init(&omap4_sdp4430_wlan_data, GPIO_WIFI_IRQ);
if (ret)
- pr_err("Error setting wl12xx data: %d\n", ret);
+ return;
+
ret = platform_device_register(&omap_vwlan_device);
if (ret)
pr_err("Error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3evm.c
b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..8e3a075 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -636,10 +636,10 @@ static void __init omap3_evm_wl12xx_init(void)
int ret;
/* WL12xx WLAN Init */
- omap3evm_wlan_data.irq = gpio_to_irq(OMAP3EVM_WLAN_IRQ_GPIO);
- ret = wl12xx_set_platform_data(&omap3evm_wlan_data);
+ ret = wl12xx_board_init(&omap3evm_wlan_data, OMAP3EVM_WLAN_IRQ_GPIO);
if (ret)
- pr_err("error setting wl12xx data: %d\n", ret);
+ return;
+
ret = platform_device_register(&omap3evm_wlan_regulator);
if (ret)
pr_err("error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3pandora.c
b/arch/arm/mach-omap2/board-omap3pandora.c
index 00a1f4a..bfdc7aa 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -543,13 +543,10 @@ static void __init pandora_wl1251_init(void)
if (ret < 0)
goto fail;
- pandora_wl1251_pdata.irq = gpio_to_irq(PANDORA_WIFI_IRQ_GPIO);
- if (pandora_wl1251_pdata.irq < 0)
- goto fail_irq;
-
pandora_wl1251_pdata.use_eeprom = true;
- ret = wl12xx_set_platform_data(&pandora_wl1251_pdata);
- if (ret < 0)
+
+ ret = wl12xx_board_init(&pandora_wl1251_pdata, PANDORA_WIFI_IRQ_GPIO);
+ if (ret)
goto fail_irq;
return;
diff --git a/arch/arm/mach-omap2/board-omap4panda.c
b/arch/arm/mach-omap2/board-omap4panda.c
index bfcd397..f203cd9 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -486,24 +486,32 @@ static void omap4_panda_init_rev(void)
}
}
+static void __init omap4_panda_wlan_init(void)
+{
+ int ret;
+
+ ret = wl12xx_board_init(&omap
Re: Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)
Hi Igor,
On Wed, Oct 17, 2012 at 2:43 PM, Igor Grinberg wrote:
> You are adding declarations inside the common-board-devices.h,
> but the implementation inside the devices.c.
> I can see that devices.c has only on-soc devices in it.
> So, I would expect to have the wl12xx_board_init() function implementation
> inside the common-board-devices.c file.
I really don't mind. Tony do you have any preference?
> Another minor nit: I don't think you need to mark the declaration as __init,
> only the implementation, or is it for documenting it so?
It may be, but I don't really mind removing it. Let's remove it if
we'll move to common-board-devices.c, otherwise it probably isn't
worth the noise.
> Instead of the above, wouldn't it be better to have:
> #if defined(CONFIG_WL12XX_PLATFORM_DATA)
> int __init wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int
> irq_gpio)
> {
> ...
> }
> #else
> inline int wl12xx_board_init(struct wl12xx_platform_data *wlan_data, int
> irq_gpio)
> {
> return 0;
> }
> #endif
I think readability-wise we're probably better off without the #ifdef.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)
On Tue, Oct 16, 2012 at 8:26 PM, Tony Lindgren wrote:
> Hmm looking at it repeats the same code over again. Can you
> rather add some wl12xx_board_init() helper function to mach-omap2/devices.c
> to do it?
Nice, see below. Note that I can only compile test this now, which may
be ok because it's pretty trivial. But do let me know if you want me
to get it tested.
>From b940fb88a97494ad3a0a093279a5f176c0b29e44 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen
Date: Sun, 14 Oct 2012 20:16:01 +0200
Subject: [PATCH] ARM: OMAP: don't print any error when wl12xx isn't
configured
Stop intimidating users with scary wlan error messages in case wl12xx
support wasn't even built.
In addition, when wl12xx_set_platform_data() fails, don't bother
registering wl12xx's fixed regulator device (on the relevant
boards).
While we're at it, extract the wlan init code to a dedicated function to
make (the relevant boards') init code look a bit nicer.
Compile tested only.
Reported-by: Russell King
Signed-off-by: Ohad Ben-Cohen
---
arch/arm/mach-davinci/board-da850-evm.c | 8 ++--
arch/arm/mach-omap2/board-4430sdp.c | 7 ---
arch/arm/mach-omap2/board-omap3evm.c | 6 +++---
arch/arm/mach-omap2/board-omap3pandora.c | 9 +++--
arch/arm/mach-omap2/board-omap4panda.c | 20 ++--
arch/arm/mach-omap2/board-zoom-peripherals.c | 15 ++-
arch/arm/mach-omap2/common-board-devices.h | 2 ++
arch/arm/mach-omap2/devices.c| 26 ++
8 files changed, 64 insertions(+), 29 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da850-evm.c
b/arch/arm/mach-davinci/board-da850-evm.c
index 32ee3f8..7243c22 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1401,13 +1401,9 @@ static __init int da850_wl12xx_init(void)
goto free_wlan_en;
}
- da850_wl12xx_wlan_data.irq = gpio_to_irq(DA850_WLAN_IRQ);
-
- ret = wl12xx_set_platform_data(&da850_wl12xx_wlan_data);
- if (ret) {
- pr_err("Could not set wl12xx data: %d\n", ret);
+ ret = wl12xx_board_init(&da850_wl12xx_wlan_data, DA850_WLAN_IRQ);
+ if (ret)
goto free_wlan_irq;
- }
return 0;
diff --git a/arch/arm/mach-omap2/board-4430sdp.c
b/arch/arm/mach-omap2/board-4430sdp.c
index 3669c12..bc48679 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -825,10 +825,11 @@ static void __init omap4_sdp4430_wifi_init(void)
int ret;
omap4_sdp4430_wifi_mux_init();
- omap4_sdp4430_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
- ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
+
+ ret = wl12xx_board_init(&omap4_sdp4430_wlan_data, GPIO_WIFI_IRQ);
if (ret)
- pr_err("Error setting wl12xx data: %d\n", ret);
+ return;
+
ret = platform_device_register(&omap_vwlan_device);
if (ret)
pr_err("Error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3evm.c
b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..8e3a075 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -636,10 +636,10 @@ static void __init omap3_evm_wl12xx_init(void)
int ret;
/* WL12xx WLAN Init */
- omap3evm_wlan_data.irq = gpio_to_irq(OMAP3EVM_WLAN_IRQ_GPIO);
- ret = wl12xx_set_platform_data(&omap3evm_wlan_data);
+ ret = wl12xx_board_init(&omap3evm_wlan_data, OMAP3EVM_WLAN_IRQ_GPIO);
if (ret)
- pr_err("error setting wl12xx data: %d\n", ret);
+ return;
+
ret = platform_device_register(&omap3evm_wlan_regulator);
if (ret)
pr_err("error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3pandora.c
b/arch/arm/mach-omap2/board-omap3pandora.c
index 00a1f4a..bfdc7aa 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -543,13 +543,10 @@ static void __init pandora_wl1251_init(void)
if (ret < 0)
goto fail;
- pandora_wl1251_pdata.irq = gpio_to_irq(PANDORA_WIFI_IRQ_GPIO);
- if (pandora_wl1251_pdata.irq < 0)
- goto fail_irq;
-
pandora_wl1251_pdata.use_eeprom = true;
- ret = wl12xx_set_platform_data(&pandora_wl1251_pdata);
- if (ret < 0)
+
+ ret = wl12xx_board_init(&pandora_wl1251_pdata, PANDORA_WIFI_IRQ_GPIO);
+ if (ret)
goto fail_irq;
return;
diff --git a/arch/arm/mach-omap2/board-omap4panda.c
b/arch/arm/mach-omap2/board-omap4panda.c
index bfcd397..f203cd9 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-oma
Re: Errors at boot time from OMAP4430SDP (and a whinge about serial stuff)
On Fri, Oct 12, 2012 at 6:24 PM, Tony Lindgren wrote:
>> Error setting wl12xx data: -38
..
> Ohad, can you please take a look?
Sure, -38 is -ENOSYS which is returned when the wl12xx driver isn't configured.
This isn't an error (it's a user decision, and it shouldn't elicit any
error) and the patch below (also attached) should make it go away on:
>From 374f145568585c8d6a8d5e4b8b5d3e6baedd2f39 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen
Date: Sun, 14 Oct 2012 20:16:01 +0200
Subject: [PATCH] ARM: OMAP: don't print any error when wl12xx isn't
configured
Stop intimidating users with scary wlan error messages in case wl12xx
support wasn't even built.
In addition, when wl12xx_set_platform_data() fails, don't bother
registering wl12xx's fixed regulator device (on the relevant
boards).
While we're at it, extract the wlan init code to a dedicated function to
make (the relevant boards') init code look a bit nicer.
Reported-by: Russell King
Signed-off-by: Ohad Ben-Cohen
---
arch/arm/mach-davinci/board-da850-evm.c | 7 ++-
arch/arm/mach-omap2/board-4430sdp.c | 13 ++--
arch/arm/mach-omap2/board-omap3evm.c | 11 +-
arch/arm/mach-omap2/board-omap3pandora.c | 9 -
arch/arm/mach-omap2/board-omap4panda.c | 30 ++--
arch/arm/mach-omap2/board-zoom-peripherals.c | 20 ---
6 files changed, 76 insertions(+), 14 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da850-evm.c
b/arch/arm/mach-davinci/board-da850-evm.c
index 32ee3f8..1b19415 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1404,8 +1404,13 @@ static __init int da850_wl12xx_init(void)
da850_wl12xx_wlan_data.irq = gpio_to_irq(DA850_WLAN_IRQ);
ret = wl12xx_set_platform_data(&da850_wl12xx_wlan_data);
+ /* bail out silently in case wl12xx isn't configured */
+ if (ret == -ENOSYS)
+ goto free_wlan_irq;
+
+ /* bail out verbosely on any other error */
if (ret) {
- pr_err("Could not set wl12xx data: %d\n", ret);
+ pr_err("error setting wl12xx data: %d\n", ret);
goto free_wlan_irq;
}
diff --git a/arch/arm/mach-omap2/board-4430sdp.c
b/arch/arm/mach-omap2/board-4430sdp.c
index 3669c12..d1dd81a 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -826,9 +826,18 @@ static void __init omap4_sdp4430_wifi_init(void)
omap4_sdp4430_wifi_mux_init();
omap4_sdp4430_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
+
ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
- if (ret)
- pr_err("Error setting wl12xx data: %d\n", ret);
+ /* bail out silently in case wl12xx isn't configured */
+ if (ret == -ENOSYS)
+ return;
+
+ /* bail out verbosely on any other error */
+ if (ret) {
+ pr_err("error setting wl12xx data: %d\n", ret);
+ return;
+ }
+
ret = platform_device_register(&omap_vwlan_device);
if (ret)
pr_err("Error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3evm.c
b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..7f72e44 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -637,9 +637,18 @@ static void __init omap3_evm_wl12xx_init(void)
/* WL12xx WLAN Init */
omap3evm_wlan_data.irq = gpio_to_irq(OMAP3EVM_WLAN_IRQ_GPIO);
+
ret = wl12xx_set_platform_data(&omap3evm_wlan_data);
- if (ret)
+ /* bail out silently in case wl12xx isn't configured */
+ if (ret == -ENOSYS)
+ return;
+
+ /* bail out verbosely on any other error */
+ if (ret) {
pr_err("error setting wl12xx data: %d\n", ret);
+ return;
+ }
+
ret = platform_device_register(&omap3evm_wlan_regulator);
if (ret)
pr_err("error registering wl12xx device: %d\n", ret);
diff --git a/arch/arm/mach-omap2/board-omap3pandora.c
b/arch/arm/mach-omap2/board-omap3pandora.c
index 00a1f4a..bbc4001 100644
--- a/arch/arm/mach-omap2/board-omap3pandora.c
+++ b/arch/arm/mach-omap2/board-omap3pandora.c
@@ -549,8 +549,15 @@ static void __init pandora_wl1251_init(void)
pandora_wl1251_pdata.use_eeprom = true;
ret = wl12xx_set_platform_data(&pandora_wl1251_pdata);
- if (ret < 0)
+ /* bail out silently in case wl12xx isn't configured */
+ if (ret == -ENOSYS)
+ goto fail_irq;
+
+ /* bail out verbosely on any other error */
+ if (ret) {
+ pr_err("error setting wl12xx data: %d\n", ret);
goto fail_irq;
+
