Message-ID: <20131009214046.GA2322@kartoffel>
Date:	Wed, 9 Oct 2013 22:40:46 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Suman Anna <s-anna@...com>
Cc:	Kumar Gala <galak@...eaurora.org>,
	Ohad Ben-Cohen <ohad@...ery.com>,
	Tony Lindgren <tony@...mide.com>,
	Benoit Cousson <bcousson@...libre.com>,
	Paul Walmsley <paul@...an.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCHv2 1/9] hwspinlock/core: add common dt bindings and OF
 helpers

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@...com>
> >>> ---
> >>> .../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@...ery.com>
> >>>  *
> >>> @@ -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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists