Re: [PATCH v8 2/4] hwspinlock/core: add device tree support

2015-03-12 Thread Bjorn Andersson 
On Wed 04 Mar 18:01 PST 2015, Suman Anna wrote:

 This patch adds a new OF-friendly API of_hwspin_lock_get_id()
 for hwspinlock clients to use/request locks from a hwspinlock
 device instantiated through a device-tree blob. This new API
 can be used by hwspinlock clients to get the id for a specific
 lock using the phandle + args specifier, so that it can be
 requested using the available hwspin_lock_request_specific()
 API.
 
 Signed-off-by: Suman Anna s-a...@ti.com

Reviewed-by: Bjorn Andersson bjorn.anders...@sonymobile.com

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2015-02-16 Thread Bjorn Andersson 
On Wed, Feb 11, 2015 at 3:29 AM, Mark Rutland mark.rutl...@arm.com wrote:
 On Thu, Jan 29, 2015 at 03:58:42AM +, Suman Anna wrote:
 On 01/22/2015 12:56 PM, Mark Rutland wrote:
[..]
  That's the only way I would expect this to possibly remain a stable
  over time, and it's the entire reason for #hwlock-cells, no?
 
  How do you expect the other components sharing the hwspinlocks to be
  described?

 Yes indeed, this is what any of the clients will use on Linux. But
 this is not necessarily the semantics for exchanging hwlocks with the
 other processor(s) which is where the global id space comes into
 picture.

 I did try to consider that above. Rather than thinking about the
 numbering as global, think of it as unique within the a given pool
 shared between processors. That's what the poolN names are about
 above.

 That way you can dynamically allocate within the pool and know that
 Linux and the SW on the other processors will use the same ID. You can
 have pools that span multiple hwlock hardware blocks, and you can have
 multiple separate pools in operation at once.

 Surely that covers the cases you care about?

 If using names is clunky, we could instead have a pool-hwlocks property
 for that purpose.


Just to make I understand your suggestion.

We would have the communication entity list all the potential hwlocks
(and gpios etc) that it can share and the key to be communicated would
then basically be the index in that list?

Like:
awesome-hub {
pool-hwlocks = a 1, a 3, b 5;
};

And a communicated lock 2 would mean lock 3 from block a?


This would make it possible to describe what locks are available in
this allocation pool and would keep such allocation logic out from
the hwlock core - as the awesome-hub driver could simply trial and
error (with some logic) through the list.

Is this understanding correct?

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2015-02-16 Thread Bjorn Andersson 
On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen o...@wizery.com wrote:
[..]
 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?


It turns out that the Qualcomm platforms have two additional remote
spinlock mechanisms - both using shared memory.

On most platforms only 1 (out of these 3) is actually consumed, but
e.g. the older msm7x30 uses 2 of them (SMEM and DAL). Due to its age I
don't think it's on anyones todo list to actually support this
platform as of today; but it's out there.


None of these hwlocks are allocated dynamically, so if needed a
dynamic base_id (-1 like other frameworks) would be a acceptable
solution - and can easily be implemented when we need it.

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2015-02-05 Thread Bjorn Andersson 
On Mon, Feb 2, 2015 at 1:07 PM, Suman Anna s-a...@ti.com wrote:
 On 02/01/2015 11:55 AM, Bjorn Andersson wrote:
 On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen o...@wizery.com wrote:
 On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson bj...@kryo.se 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.


 Correct, so the #hwlock-cells and hwlock part from the proposal are
 the important one. Having an optional hwlock-names will make things
 easier to read as well, but is not necessary.

 Right, if anything, it would be useful only for the clients, but the
 hwspinlock core itself would not need it. So, I would forgo adding the
 hwlock-names for now.


 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.


 From the dt binding PoV, we should be able to skip num-locks as well.
 It seems most hwlock blocks have a fixed amount of locks provided and
 the drivers are reporting this to the core when registering.

 I added this originally based on the initial MSM HW Mutex block bindings.


It's not entirely correct to have this in DT for the MSM HW, as the
hardware has a fixed number of mutexes. As soon as we have the binding
sorted out I will follow up with a new revision of the tcsr/sfpb-mutex
driver.


 So I think we can reduce the binding to:

 Providers:
 #hwlock-cells

 Consumers:
 hwlocks
 hwlock-names

 For the hardware where number of locks is actually variable (e.g.
 different variants of same block) there can be driver specific entries
 for this.

 Right, we should be able to drop this and use the driver match data. As
 it is, the field is used during registration of the block with the
 hwspinlock core.


If we have certain systems where it actually is a property to be
configured then they can have individual properties, extending the
standard set. Either way, it's not a dynamic property shared by all
hwlock drivers, so it should not be in the common binding.

Will you send out a new revision of the binding? I would love to get
this integrated so I can move on with the dependents.

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2015-02-05 Thread Bjorn Andersson 
On Thu, Feb 5, 2015 at 4:11 PM, Suman Anna s-a...@ti.com wrote:
 Hi Bjorn,

 On 02/05/2015 05:01 PM, Bjorn Andersson wrote:
[..]
 Will you send out a new revision of the binding? I would love to get
 this integrated so I can move on with the dependents.

 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 :/

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2015-02-01 Thread Bjorn Andersson 
On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen o...@wizery.com wrote:
 On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson bj...@kryo.se 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.


Correct, so the #hwlock-cells and hwlock part from the proposal are
the important one. Having an optional hwlock-names will make things
easier to read as well, but is not necessary.

 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.


From the dt binding PoV, we should be able to skip num-locks as well.
It seems most hwlock blocks have a fixed amount of locks provided and
the drivers are reporting this to the core when registering.

So I think we can reduce the binding to:

Providers:
#hwlock-cells

Consumers:
hwlocks
hwlock-names

For the hardware where number of locks is actually variable (e.g.
different variants of same block) there can be driver specific entries
for this.

 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?


I haven't seen any such systems yet.

We could introduce the logic found in other subsystems of allowing -1
as base_id and having the core find a available range. This will work
for all cases where the global ids doesn't have to be static; either
due to the fact that we use block:local-id or that the indices are
hard coded. (Referencing locks via dt is equivalent of the
block:local-id case)

 - 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.


That makes more sense, although I still believe that you end up with a
system wide setup where locks are statically allocated in some
document beforehand. Either that or you will have to feed that other
system with the list of constraints.

Non the less, that's unrelated.

If we get an incoming message saying lckX:Y (or the global lckZ), we
probably wouldn't define that in DT. We would need a way to resolve
the hwlock-block lckX so we can request lock Y from that block. We
would basically build the DT reference on the fly.

I think this is a future problem to be solved and more importantly I
don't think it's limited to hwlocks. If a system architect designs a
system where a remote entity will do allocation of resources for us,
they will most likely do so not only for hwlocks but also for gpios,
irqs and other resources that we today reference with arguments in DT.
Hopefully that will not happen anytime soon, so let's just ignore that
problem for now and go for a simple binding that will cover todays use
cases (with some thought into multi-block support).

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2015-01-30 Thread Bjorn Andersson 
On Fri, Jan 16, 2015 at 4:46 PM, Ohad Ben-Cohen o...@wizery.com wrote:
 Mark,

 On Fri, Jan 16, 2015 at 12:17 PM, Mark Rutland mark.rutl...@arm.com 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.


But that's exactly how irqs or gpios work as well. If you have 32
gpios in a system they used to be numbered 0-31 and people would
reference them directly by that number. Every one of the systems that
was designed in this way is moving away from it.

 - 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).

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;
will you tell the firmware engineers to always subtract 8 from the
numbers you pass them?

Wouldn't it make much more sense to have local indexes here and pass
them e.g lckB:2?

 - 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.

Further more this dynamic allocation leads to interesting race
conditions as what happens if you dynamically allocate a hwlock that
is statically allocated by another part of the system?
The only solution I can think of is to have a static allocation of ids
that the dynamic allocator might use, and then we're just carrying
extra code when the system is already statically configured...

 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.


Designing this interface to take into consideration that someone might
send us something completely crazy isn't productive.


The only reason for having num-locks and base-id in device tree is
because of the current Linux implementation. base-id is not a property
of the hardware and num-locks is not needed for anything but book
keeping of base-id's in the hwlock framework.

This is why I preferred Sumans earlier suggestion of having the
binding consist of #hwlock-cells = X and the necessary accessor
functions for resolving a hwlock based on a dt reference.

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2014-12-08 Thread Bjorn Andersson 
On Wed, Nov 12, 2014 at 9:08 AM, Suman Anna s-a...@ti.com wrote:
 Kumar, Mark, Rob,

 On 11/12/2014 09:14 AM, Ohad Ben-Cohen wrote:
 Hi Suman,

 On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna s-a...@ti.com 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 robh...@kernel.org
 Signed-off-by: Suman Anna s-a...@ti.com

Reviewed-by: Bjorn Andersson bjorn.anders...@sonymobile.com

 ---
  .../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.


 Can one of you ack this patch and the next patch [1] so that Ohad can
 pick up the series for 3.19? This series has been in works for quite
 some time now and was reviewed by each of you at some point of time
 (thanks!!), the cover letter [2] has the history of the changes.


Could we please have a statement from a DT maintainer so we could move
forward with this?

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2014-11-19 Thread Bjorn Andersson 
On Wed, Nov 12, 2014 at 11:14 AM, Ohad Ben-Cohen o...@wizery.com wrote:
 Hi Suman,
[..]

 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.


Hi Ohad,

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?

What systems use more than one hwlock block and do you know of any
reasons why these hwlocks are globally numbered?

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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 0/5] hwspinlock core/omap dt support

2014-09-30 Thread Bjorn Andersson 
On Fri, Sep 12, 2014 at 1:24 PM, Suman Anna s-a...@ti.com wrote:
 Hi Ohad,

 This is an update to the hwspinlock dt support series. The series
 is rebased onto v3.17-rc3, and addresses the review comments on the
 previous v5 series. I have also split and left out the RFC patches
 about the support for reserved locks (will post these as a separate
 series) and return code convention changes in the hwspinlock core
 (will not be needed anymore). The support for deferred probing of
 clients is supported in the new of_hwspin_lock_get_id() function
 itself.


Thanks for your reply to me, I had missed that you continued this work.


I find it somewhat awkward to have to call both of_hwspin_lock_get_id() and
then hwspin_lock_request_specific(), but I found the request from Ohad, so
let's stick with it.


Am I right that hwlock-num-locks and hwlock-base-id are optional from the
frameworks perspective and only there to aid the hwspin drivers? If so it is
strange to have in the common binding and have the helper functions in the core
for simply reading hwspin device specific properties.

Otherwise I think it looks sane, although I haven't spend that much time
reviewing it.

I did throw it into my tree and gave it a testrun with the Qualcomm code I've
been working on. So for the non-omap parts:

Tested-by: Bjorn Andersson bjorn.anders...@sonymobile.com

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2014-09-26 Thread Bjorn Andersson 
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna s-a...@ti.com wrote:
 This patch adds three new OF helper functions to use/request
 locks from a hwspinlock device instantiated through a
 device-tree blob.


Hi Ohad, Suman

I'm about to send out some patches that depends on this functionality,
how do we move forward?

I still think it's wrong to not return -EPROBE_DEFER, but I much
rather have the code returning NULL than not having it in the tree (we
can always argue about it later...).

@Suman, do you remember if there was any other comments on the patch?

@Ohad, do you object merging Suman's patch in it's current form? I
think it should still apply cleanly.

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2014-03-02 Thread Bjorn Andersson 
On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen o...@wizery.com wrote:
 On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna s-a...@ti.com 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.

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.

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe 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

2014-02-07 Thread Bjorn Andersson 
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna s-a...@ti.com wrote:
 This patch adds three new OF helper functions to use/request
 locks from a hwspinlock device instantiated through a
 device-tree blob.

Nice, I ran in to the problem of needing a probe deferral on a
hwspinlock earlier this week so I implemented this yesterday...then I
got a pointer to your series.

[snip]
  /**
 + * of_hwspin_lock_request_specific() - request a OF phandle-based specific 
 lock
 + * @np: device node from which to request the specific hwlock
 + * @propname: property name containing hwlock specifier(s)
 + * @index: index of the hwlock
 + *
 + * This function is the OF equivalent of hwspin_lock_request_specific(). This
 + * function provides a means for users of the hwspinlock module to request a
 + * specific hwspinlock using the phandle of the hwspinlock device. The 
 requested
 + * lock number is indexed relative to the hwspinlock device, unlike the
 + * hwspin_lock_request_specific() which is an absolute lock number.
 + *
 + * Returns the address of the assigned hwspinlock, or NULL on error
 + */
 +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
 +   const char *propname, int index)
 +{
 +   struct hwspinlock_device *bank;
 +   struct of_phandle_args args;
 +   int id;
 +   int ret;
 +
 +   ret = of_parse_phandle_with_args(np, propname, #hwlock-cells, index,
 +args);
 +   if (ret) {
 +   pr_warn(%s: can't parse hwlocks property of node '%s[%d]' 
 ret = %d\n,
 +   __func__, np-full_name, index, ret);
 +   return NULL;
 +   }

of_parse_phandle_with_args() already does pr_err if it can't find the
phandle and on some of the issues related to arguments. So please
remove this pr_warn().

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...

 +
 +   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) {
 +   pr_warn(%s: requested hwspinlock device %s is not 
 registered\n,
 +   __func__, args.np-full_name);
 +   return NULL;

...especially since you want the consumer to have the ability to
identify this error. Here you should
return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this
lock is not _yet_ registered, but will be in the future.

You should remove this pr_warn as well. The standard use of this
function would be in a probe() and just returning this error value
from that probe will give you a line in the log indicating that this
was in fact the issue.

 +   }
 +
 +   id = bank-ops-of_xlate(bank, args);
 +   if (id  0 || id = bank-num_locks) {
 +   pr_warn(%s: requested lock %d is either out of range [0, %d] 
 or failed translation\n,
 +   __func__, id, bank-num_locks - 1);
 +   return NULL;

Please return ERR_PTR(-EINVAL); here.


Looking forward to your next spin, as I will actually use this interface :)

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html