On Thu, Oct 03, 2013 at 05:04:15AM +0100, Suman Anna wrote: > Hi Mark, > > On 10/01/2013 03:36 AM, Mark Rutland wrote: > > Hi Suman, > > > > Apologies for replying to a subthread, due to an earlier mistake on my > > part I don't have the original to hand. > > No issues, thanks for your review. > > > > > On Fri, Sep 27, 2013 at 05:04:22PM +0100, Kumar Gala wrote: > >> > >> On Sep 17, 2013, at 2:30 PM, Suman Anna wrote: > >> > >>> All the platform-specific hwlock driver implementations need the > >>> number of locks and the associated base id for registering the > >>> locks present within a hwspinlock device with the driver core. > >>> These two variables are represented by 'hwlock-num-locks' and > >>> 'hwlock-base-id' properties. The documentation and OF helpers to > >>> retrieve these common properties have been added to the driver core. > >>> > >>> Signed-off-by: Suman Anna <s-anna@xxxxxx> > >>> --- > >>> .../devicetree/bindings/hwlock/hwlock.txt | 26 +++++++++ > >>> drivers/hwspinlock/hwspinlock_core.c | 61 +++++++++++++++++++++- > >>> include/linux/hwspinlock.h | 11 ++-- > >>> 3 files changed, 93 insertions(+), 5 deletions(-) > >>> create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/hwlock/hwlock.txt b/Documentation/devicetree/bindings/hwlock/hwlock.txt > >>> new file mode 100644 > >>> index 0000000..789930e > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/hwlock/hwlock.txt > >>> @@ -0,0 +1,26 @@ > >>> +Generic hwlock bindings > >>> +======================= > >>> + > >>> +Generic bindings that are common to all the hwlock platform specific driver > >>> +implementations, the retrieved values are used for registering the device > >>> +specific parameters with the hwspinlock core. > >>> + > >>> +The validity and need of these common properties may vary from one driver > >>> +implementation to another. Look through the individual hwlock driver > >>> +binding documentations for identifying which are mandatory and which are > >>> +optional for that specific driver. > >>> + > >>> +Common properties: > >>> +- hwlock-base-id: Base Id for the locks for a particular hwlock device. > >>> + This property is used for representing a set of locks > >>> + present in a hwlock device with a unique base id in > >>> + the driver core. This property is mandatory ONLY if a > >>> + SoC has several hwlock devices. > >>> + > >>> + See documentation on struct hwspinlock_pdata in > >>> + linux/hwspinlock.h for more details. > > > > I don't like this, as it seems to be encoding a Linux implementation > > detail (the ID of the lock, which means that we have to have a numeric > > representation of each hwlock) in the device tree. > > > > I don't see why the ID within Linux should be a concern of the device > > tree binding. I think that whatever internal identifier we have in Linux > > (be it an integer or struct) should be allocated by Linux. If a driver > > needs to request specific hwlocks, then we should have a phandle + args > > representation for referring to a specific hwlock that bidnings can use, > > and some code for parsing that and returning a Linux-internal > > identifier/struct as we do with interrupts and clocks. > > This is based on gathering the information required by the platform > implementation drivers for registering with the driver core. The driver > core currently maintains all the locks from different instances as a > radix tree, as it is simpler to manage when granting locks to users. > The current version is based on allowing the platform implementation > drivers to retrieve the required data for registering with the > hwspinlock driver core. Ok. I don't see why this implementation detail needs to leak into the dt. > > The users would either get a lock dynamically by requesting any free one > (and extract the id thereafter to share with others), or a specific one > which is currently by id. I agree that the phandle + args approach is > best suited for requesting a specific one through DT, but I would think > that the args would still have to be a relative lock number from 0 w.r.t > a phandle. I will look into the feasibility of such an approach > retaining the radix tree implementation, as this requires new OF > friendly registration and request functions. The value in the args would be a unique identifier within the unit pointed to be the phandle, but the mechanism by which it would refer to a particular lock would be binding-specific. It's perfectly fine for this to be an zero-based index in most bindings, but it should not be a global namespace as with the hwlock-base-id property approach. > > > > >>> + > >>> +- hwlock-num-locks: Number of locks present in a hwlock device. This > >>> + property is needed on hwlock devices, where the number > >>> + of supported locks within a hwlock device cannot be > >>> + read from a register. > > > > Hmm... this seems generic, but it doesn't allow for sparse ID spaces on > > the hwlock module. It should probably be common for the moment, but if > > we encounter a hwlock module with a sparse ID space, we'll need to come > > up with a way of handling sparse IDs (that might be device-specific). > > Agreed. > > > > >>> diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c > >>> index 461a0d7..aec32e7 100644 > >>> --- a/drivers/hwspinlock/hwspinlock_core.c > >>> +++ b/drivers/hwspinlock/hwspinlock_core.c > >>> @@ -1,7 +1,7 @@ > >>> /* > >>> * Hardware spinlock framework > >>> * > >>> - * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com > >>> + * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com > >>> * > >>> * Contact: Ohad Ben-Cohen <ohad@xxxxxxxxxx> > >>> * > >>> @@ -27,6 +27,7 @@ > >>> #include <linux/hwspinlock.h> > >>> #include <linux/pm_runtime.h> > >>> #include <linux/mutex.h> > >>> +#include <linux/of.h> > >>> > >>> #include "hwspinlock_internal.h" > >>> > >>> @@ -308,6 +309,64 @@ out: > >>> } > >>> > >>> /** > >>> + * of_hwspin_lock_get_base_id() - OF helper to retrieve base id > >>> + * @dn: device node pointer > >>> + * > >>> + * This is an OF helper function that can be called by the underlying > >>> + * platform-specific implementations, to retrieve the base id for the > >>> + * set of locks present within a hwspinlock device instance. > >>> + * > >>> + * Returns the base id value on success, -ENODEV on generic failure or > >>> + * an appropriate error code as returned by the OF layer > >>> + */ > >>> +int of_hwspin_lock_get_base_id(struct device_node *dn) > >>> +{ > >>> + unsigned int val; > >>> + int ret = -ENODEV; > >>> + > >>> +#ifdef CONFIG_OF > >>> + if (!dn) > >>> + return -ENODEV; > >> > >> Checking !dn is done in of_property_read_u32, so you don't need to duplicate > >> > >>> + > >>> + ret = of_property_read_u32(dn, "hwlock-base-id", &val); > >>> + if (!ret) > >>> + ret = val; > >>> +#endif > > > > Do we need the CONFIG_OF check? of_property_read_u32 is defined to > > return -ENOSYS if CONFIG_OF isn't defined. Or would that confuse a > > higher layer? > > These are primarily OF helpers and provided for the SoC implementation > drivers, and I have used the CONFIG_OF check within the function to > streamline the function prototypes and behavior in the common > hwspinlock.h header file between combinations of CONFIG_HWSPINLOCK and > CONFIG_OF. Ok. Due to the !CONFIG_OF stub for of_property_read_u32, and the check for !dn done in of_property_read_u32, you could just have: int of_hwspin_lock_get_base_id(struct device_node *dn) { u32 val; if (of_property_read_u32(dn, "hwlock-base-id", &val) != 0) return -ENODEV; return val; } Which would work regardless of CONFIG_OF. That said, I don't think this function is necessary because a phandle + args approach would be fundamantally better. > > The check for !dn is done deliberately to help the implementation drivers. As I mentioned above, of_property_read_u32 already checks for !dn, so the check here is redundant. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
