From: Srinivas Kandagatla <[email protected]>
This patch introduces syscon_claim, syscon_read, syscon_write,
syscon_release APIs to help drivers to use syscon registers in much more
flexible way.
With this patch, a driver can claim few/all bits in the syscon registers
and do read/write and then release them when its totally finished with
them, in the mean time if another driver requests same bits or registers
the API will detect conflit and return error to the second request.
Reason to introduce this API.
System configuration/control registers are very basic configuration
registers arranged in groups across ST Settop Box parts. These registers
are independent of IP itself. Many IPs, clock, pad and other functions
are wired up to these registers.
In many cases a single syconf register contains bits related to multiple
devices, and therefore it need to be shared across multiple drivers at
bit level. The same IP block can have different syscon mappings on
different SOCs.
Typically in a SOC there will be more than hundreds of these registers,
which are again divided into groups.
Signed-off-by: Srinivas Kandagatla <[email protected]>
CC: Stuart Menefy <[email protected]>
---
drivers/mfd/syscon.c | 199 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/syscon.h | 43 ++++++++++
2 files changed, 242 insertions(+), 0 deletions(-)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 61aea63..fab85da 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -16,6 +16,7 @@
#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/slab.h>
#include <linux/of_address.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
@@ -28,8 +29,23 @@ struct syscon {
struct device *dev;
void __iomem *base;
struct regmap *regmap;
+ struct device_node *of_node;
+ struct list_head list;
+ struct list_head fields;
+ spinlock_t fields_lock;
};
+struct syscon_field {
+ struct syscon *syscon;
+ u16 num;
+ u8 lsb, msb;
+ unsigned int offset;
+ const char *owner;
+ struct list_head list;
+};
+
+static LIST_HEAD(syscons);
+
static int syscon_match(struct device *dev, void *data)
{
struct syscon *syscon = dev_get_drvdata(dev);
@@ -87,6 +103,185 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
+static int syscon_add_field(struct syscon_field *field)
+{
+ struct syscon_field *entry;
+ enum {
+ status_not_found,
+ status_found_register,
+ status_add_field_here,
+ } status = status_not_found;
+ int bit_avail = 0;
+ struct syscon *syscon = field->syscon;
+ int rval = 0;
+ int num = field->num;
+ int lsb = field->lsb;
+ int msb = field->msb;
+
+ spin_lock(&syscon->fields_lock);
+ /*
+ * The list is always in syscon->num->lsb/msb order, so it's easy to
+ * find a place to insert a new field (and to detect conflicts)
+ */
+ list_for_each_entry(entry, &syscon->fields, list) {
+ if (entry->num == num) {
+ status = status_found_register;
+ /*
+ * Someone already claimed a field from this
+ * register - let's try to find some space for
+ * requested bits...
+ */
+ if (bit_avail <= lsb && msb < entry->lsb) {
+ status = status_add_field_here;
+ break;
+ }
+ bit_avail = entry->msb + 1;
+ } else if (entry->num > num) {
+ /*
+ * There is no point of looking further -
+ * the num values are bigger then
+ * the ones we are looking for
+ */
+ if ((status == status_found_register &&
+ bit_avail <= lsb) ||
+ status == status_not_found)
+ /*
+ * A remainder of the given register is not
+ * used or the register wasn't used at all
+ */
+ status = status_add_field_here;
+ else
+ /*
+ * Apparently some bits of the claimed field
+ * are already in use...
+ */
+ rval = -EBUSY;
+ break;
+ }
+ }
+
+ if ((status == status_not_found) || (status == status_found_register))
+ list_add_tail(&field->list, &syscon->fields);
+ else if (status == status_add_field_here)
+ list_add(&field->list, entry->list.prev);
+
+ spin_unlock(&syscon->fields_lock);
+ return rval;
+}
+
+static struct syscon *find_syscon(struct device_node *np)
+{
+ struct syscon *syscon;
+
+ list_for_each_entry(syscon, &syscons, list) {
+ if (syscon->of_node == np)
+ return syscon;
+ }
+ return NULL;
+}
+
+#define MIN_SYSCON_CELLS (4)
+
+struct syscon_field *syscon_claim(struct device_node *np,
+ const char *prop)
+{
+ const __be32 *list;
+ const struct property *pp;
+ struct syscon_field *field;
+ phandle phandle;
+ struct device_node *syscon_np;
+ u32 syscon_num;
+
+ pp = of_find_property(np, prop, NULL);
+ if (!pp)
+ return NULL;
+
+ if (pp->length < ((MIN_SYSCON_CELLS) * sizeof(*list)))
+ return NULL;
+
+ list = pp->value;
+
+ /* syscon */
+ phandle = be32_to_cpup(list++);
+ syscon_np = of_find_node_by_phandle(phandle);
+ if (!syscon_np) {
+ pr_warn("No syscon found for %s syscon\n", prop);
+ return NULL;
+ }
+
+ field = kzalloc(sizeof(struct syscon_field), GFP_KERNEL);
+ if (!field)
+ return NULL;
+
+ field->syscon = find_syscon(syscon_np);
+ if (!field->syscon) {
+ pr_warn("No syscon registered for %s syscon\n", prop);
+ goto error;
+ }
+
+ of_node_put(syscon_np);
+ syscon_num = be32_to_cpup(list++);
+ field->offset = (syscon_num) * 4;
+ field->lsb = be32_to_cpup(list++);
+ field->msb = be32_to_cpup(list++);
+ field->num = syscon_num;
+ field->owner = pp->name;
+
+ if (syscon_add_field(field))
+ goto error;
+
+ return field;
+error:
+ kfree(field);
+ return NULL;
+}
+EXPORT_SYMBOL(syscon_claim);
+
+void syscon_release(struct syscon_field *field)
+{
+ struct syscon *syscon;
+ if (field) {
+ syscon = field->syscon;
+ spin_lock(&syscon->fields_lock);
+ list_del(&field->list);
+ spin_unlock(&syscon->fields_lock);
+ kfree(field);
+ }
+}
+EXPORT_SYMBOL(syscon_release);
+
+void syscon_write(struct syscon_field *field, unsigned long value)
+{
+ int field_bits;
+ struct syscon *syscon = field->syscon;
+
+ field_bits = field->msb - field->lsb + 1;
+ if (field_bits == 32) {
+ regmap_write(syscon->regmap, field->offset, value);
+ } else {
+ u32 reg_mask;
+ reg_mask = (((1 << field_bits) - 1) << field->lsb);
+ regmap_update_bits(syscon->regmap, field->offset,
+ reg_mask, value << field->lsb);
+ }
+}
+EXPORT_SYMBOL(syscon_write);
+
+unsigned long syscon_read(struct syscon_field *field)
+{
+ int field_bits;
+ u32 result;
+ struct syscon *syscon = field->syscon;
+
+ regmap_read(syscon->regmap, field->offset, &result);
+ field_bits = field->msb - field->lsb + 1;
+ result >>= field->lsb;
+ result &= (1 << field_bits) - 1;
+
+ return result;
+}
+EXPORT_SYMBOL(syscon_read);
+
static const struct of_device_id of_syscon_match[] = {
{ .compatible = "syscon", },
{ },
@@ -122,6 +317,10 @@ static int syscon_probe(struct platform_device *pdev)
if (ret)
return ret;
+ list_add_tail(&syscon->list, &syscons);
+ INIT_LIST_HEAD(&syscon->fields);
+ syscon->of_node = np;
+
syscon_regmap_config.max_register = res.end - res.start - 3;
syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,
&syscon_regmap_config);
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 6aeb6b8..0de0da5 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -15,9 +15,52 @@
#ifndef __LINUX_MFD_SYSCON_H__
#define __LINUX_MFD_SYSCON_H__
+struct syscon_field;
+
extern struct regmap *syscon_node_to_regmap(struct device_node *np);
extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
extern struct regmap *syscon_regmap_lookup_by_phandle(
struct device_node *np,
const char *property);
+
+/**
+ * syscon_claim - Claim ownership of a field of a syscon register
+ * @np: parent node pointer.
+ * @prop: name of sysconf to claim.
+ *
+ * This function claims ownership of a field from a syscon register.
+ * It returns a &struct syscon_field which can be used in subsequent
+ * operations on this field.
+ */
+struct syscon_field *syscon_claim(struct device_node *np,
+ const char *prop);
+
+/**
+ * syscon_release - Release ownership of a field of a syscon register
+ * @field: the syscon field to write to
+ *
+ * Release ownership of a field from a syscon register.
+ * @field must have been claimed using syscon_claim().
+ */
+void syscon_release(struct syscon_field *field);
+
+/**
+ * syscon_write - Write a value into a field of a syscon register
+ * @field: the syscon field to write to
+ * @value: the value to write into the field
+ *
+ * This writes @value into the field of the syscon register @field.
+ * @field must have been claimed using syscon_claim().
+ */
+void syscon_write(struct syscon_field *field, unsigned long value);
+
+/**
+ * syscon_read - Read a field of a syscon register
+ * @field: the syscon field to read
+ *
+ * This reads a field of the syscon register @field.
+ * @field must have been claimed using syscon_claim().
+ */
+unsigned long syscon_read(struct syscon_field *field);
+
#endif /* __LINUX_MFD_SYSCON_H__ */
--
1.7.6.5
On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla <[email protected]>
>
> This patch introduces syscon_claim, syscon_read, syscon_write,
> syscon_release APIs to help drivers to use syscon registers in much more
> flexible way.
>
> With this patch, a driver can claim few/all bits in the syscon registers
> and do read/write and then release them when its totally finished with
> them, in the mean time if another driver requests same bits or registers
> the API will detect conflit and return error to the second request.
>
> Reason to introduce this API.
> System configuration/control registers are very basic configuration
> registers arranged in groups across ST Settop Box parts. These registers
> are independent of IP itself. Many IPs, clock, pad and other functions
> are wired up to these registers.
>
> In many cases a single syconf register contains bits related to multiple
> devices, and therefore it need to be shared across multiple drivers at
> bit level. The same IP block can have different syscon mappings on
> different SOCs.
>
> Typically in a SOC there will be more than hundreds of these registers,
> which are again divided into groups.
>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> CC: Stuart Menefy <[email protected]>
My feeling is that syscon is the wrong place for this functionality,
since regmap already handles (some of?) these issues. If you need
additional synchronization, it's probably best to extend regmap
as needed so other code besides syscon can take advantage of that
as well.
Arnd
On Wed, May 08, 2013 at 04:50:22PM +0200, Arnd Bergmann wrote:
> > In many cases a single syconf register contains bits related to multiple
> > devices, and therefore it need to be shared across multiple drivers at
> > bit level. The same IP block can have different syscon mappings on
> > different SOCs.
> My feeling is that syscon is the wrong place for this functionality,
> since regmap already handles (some of?) these issues. If you need
> additional synchronization, it's probably best to extend regmap
> as needed so other code besides syscon can take advantage of that
> as well.
This sounds like regmap_update_bits() ought to be all that's needed.
Thankyou for your comments.
On 08/05/13 15:50, Arnd Bergmann wrote:
> On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
>> From: Srinivas Kandagatla <[email protected]>
>>
>> This patch introduces syscon_claim, syscon_read, syscon_write,
>> syscon_release APIs to help drivers to use syscon registers in much more
>> flexible way.
>>
>> With this patch, a driver can claim few/all bits in the syscon registers
>> and do read/write and then release them when its totally finished with
>> them, in the mean time if another driver requests same bits or registers
>> the API will detect conflit and return error to the second request.
>>
>> Reason to introduce this API.
>> System configuration/control registers are very basic configuration
>> registers arranged in groups across ST Settop Box parts. These registers
>> are independent of IP itself. Many IPs, clock, pad and other functions
>> are wired up to these registers.
>>
>> In many cases a single syconf register contains bits related to multiple
>> devices, and therefore it need to be shared across multiple drivers at
>> bit level. The same IP block can have different syscon mappings on
>> different SOCs.
>>
>> Typically in a SOC there will be more than hundreds of these registers,
>> which are again divided into groups.
>>
>> Signed-off-by: Srinivas Kandagatla <[email protected]>
>> CC: Stuart Menefy <[email protected]>
> My feeling is that syscon is the wrong place for this functionality,
> since regmap already handles (some of?) these issues. If you need
> additional synchronization, it's probably best to extend regmap
> as needed so other code besides syscon can take advantage of that
> as well.
Its not just synchronisation that we are looking for.
It also the usability, drivers want to just refer to a syscon
register/bits of it from device trees/non-devicetrees.
The extent of syscon usage is very high in ST set-top-box parts.
As example, ST pinctrl driver uses use bits of the syscon register to
control alternate functions, and many other parameters of pinconf.
In device tree we do something like:
syscfg_sbc: syscon@fe600000{
compatible = "syscon";
reg = <0xfe600000 0xb4>;
};
and in pinctrl dts file
st,alt-control = <&syscfg_sbc 0 0 31>;
st,od-control = <&syscfg_sbc 9 0 7>;
the pinctrl driver calls syconf_claim(np, "st,alt-control) to get a
field and then do a read/write on the field.
Just in pinctrl driver we use around 50-100 sysconf registers scatters
across different groups.
Without these new APIs, its very difficult to pass this information to
the drivers.
Thanks,
srini
>
> Arnd
>
Thankyou for the comments.
On 08/05/13 16:01, Mark Brown wrote:
> On Wed, May 08, 2013 at 04:50:22PM +0200, Arnd Bergmann wrote:
>
>>> In many cases a single syconf register contains bits related to multiple
>>> devices, and therefore it need to be shared across multiple drivers at
>>> bit level. The same IP block can have different syscon mappings on
>>> different SOCs.
>> My feeling is that syscon is the wrong place for this functionality,
>> since regmap already handles (some of?) these issues. If you need
>> additional synchronization, it's probably best to extend regmap
>> as needed so other code besides syscon can take advantage of that
>> as well.
> This sounds like regmap_update_bits() ought to be all that's needed.
Ultimately the syscon_write use the regmap_update_bits, however we really want is the flexibility in using/referring the syscon registers/bits in both device-trees and non-device tree cases.
The reason for these APIs, is the extent of syscon usage is very high in ST set-top-box parts.
Without these new APIs, its very difficult to pass this information to
the drivers.
Thanks,
srini
On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
> the pinctrl driver calls syconf_claim(np, "st,alt-control) to get a
> field and then do a read/write on the field.
>
> Just in pinctrl driver we use around 50-100 sysconf registers scatters
> across different groups.
>
> Without these new APIs, its very difficult to pass this information to
> the drivers.
But are those necessarily things you would configure using DT?
If there are so many registers that are shared between mutliple
subsystems, maybe using drivers/mfd/syscon for that is taking things
further than you should, since it is unlike what any of the other
SoC families need from syscon.
Can you describe how your syscon registers are laid out?
Which subsystems beside pinctrl need to directly interact
with it? Is there any logical structure in it or do you just
have tons of bits scattered around an MMIO area?
Arnd
On Wed, May 08, 2013 at 06:42:04PM +0100, Srinivas KANDAGATLA wrote:
Fix your mailer to word wrap within paragraphs.
> Ultimately the syscon_write use the regmap_update_bits, however we
> really want is the flexibility in using/referring the syscon
> registers/bits in both device-trees and non-device tree cases.
So what you're looking for here is some way to offload discovery of
register fields from the driver?
> The reason for these APIs, is the extent of syscon usage is very high
> in ST set-top-box parts.
> Without these new APIs, its very difficult to pass this information to
> the drivers.
I'm not 100% convinced that putting all this information into DT is a
good idea, and to the extent that it is sensible it feels like something
which might be useful with any device using register maps, not just
syscon. For example many MFDs have similar needs - essentially the
system controllers are just a particular kind of MFD. To me that says
that this should be outside syscon so other things can use it.
Hi Arnd,
Thankyou for extending the discussion.
On 08/05/13 20:48, Arnd Bergmann wrote:
> On Wednesday 08 May 2013, Srinivas KANDAGATLA wrote:
>> the pinctrl driver calls syconf_claim(np, "st,alt-control) to get a
>> field and then do a read/write on the field.
>>
>> Just in pinctrl driver we use around 50-100 sysconf registers scatters
>> across different groups.
>>
>> Without these new APIs, its very difficult to pass this information to
>> the drivers.
> But are those necessarily things you would configure using DT?
In last comment.
>
> If there are so many registers that are shared between mutliple
> subsystems, maybe using drivers/mfd/syscon for that is taking things
> further than you should, since it is unlike what any of the other
> SoC families need from syscon.
I agree, my initial approach was having a dedicated driver specific to
ST syscon, however syscon seems to do things very much similar to what
we want, so I have integrated those 3 functions in syscon.
Am happy to go back with my first approach of adding ST specific syscon
driver if no one is actually going to benefit with such a change to
syscon driver.
>
> Can you describe how your syscon registers are laid out?
On STiH416 SOC we have 9 SYSCONF(aka System Configuration
Registers)named banks/groups, each bank has its own memory map.
Each sysconf bank has number of 32 bit registers which vary from bank to
bank, like sysconf bank "sbc" has range from SYSTEM_CONFIG0 to
SYSTEM_CONFIG999 where as sysconf bank "front" has range of
SYSTEM_CONFIG1000 to SYSTEM_CONFIG1999 and so on.
Each register is assigned with a unique SYCONF number, example:
SYSTEM_CONFIG100, SYSTEM_CONFIG101 , .. and so on.
Each sysconf contains bits of the IP configurations wired-up to the
sysconf register bits.
As example:
- Each pinctrl entry for set of 8 pins uses around 8-10 sysconfig
register to control pinconf and pin functions.
- IPs like Ethernet have few bit like Ethernet-Mode selection external
or internal phyclk wired up to bits in sysconf registers,
- Few clocks are controlled by sysconf registers.
- Reset to IPs are wired up to bits of sysconf same registers.
- ARM core soft reset is wired up to the sysconf registers...
And most of the IPs have similar requirements ......
Total layout of the sysconf changes per SOC, and the bit arrangements
aswell, however the core IP(pinctrl, etherenet ...) and logic to drive
those bits remains exactly same.
As the code remains the same, the information about the hardware
configuration and offsets to the registers are passed by device trees
using the node properties.
In general the requirements of the sysconf support to the SOC/driver
support is.
1> It should be able to read/write a sysconf register bits without
having to "if" each SOC in the code. So that code is totally abstracted.
Which is currently achieved by passing the information from the device
trees and the driver just uses the property to get it.
2> The infrastructure should protect the claimed registers from
over-writing by other drivers. We do this by claim-read/write-release
style API.
3> The driver should be able to set a group of sysconf registers bits to
a particular values before initialises the IP. I was thinking of doing
this in a same way as pinctrl state.
Any suggestion is most appreciated?
Thanks,
srini
> Which subsystems beside pinctrl need to directly interact
> with it? Is there any logical structure in it or do you just
> have tons of bits scattered around an MMIO area?
>
> Arnd
>
On 09/05/13 10:51, Mark Brown wrote:
> On Wed, May 08, 2013 at 06:42:04PM +0100, Srinivas KANDAGATLA wrote:
>
> Fix your mailer to word wrap within paragraphs.
>
Sorry about that.
>> Ultimately the syscon_write use the regmap_update_bits, however we
>> really want is the flexibility in using/referring the syscon
>> registers/bits in both device-trees and non-device tree cases.
>
> So what you're looking for here is some way to offload discovery of
> register fields from the driver?
Exactly,
As the System configuration registers change per each SOC, and the
logic/code to drive still remain same across SOCs.
It makes sense to abstract the information of sysconf register and its
offsets from the driver, as we do not want to change the driver per each
SOC.
>
>> The reason for these APIs, is the extent of syscon usage is very high
>> in ST set-top-box parts.
>
>> Without these new APIs, its very difficult to pass this information to
>> the drivers.
>
> I'm not 100% convinced that putting all this information into DT is a
> good idea,
Currently, we have two bits of information which come from device trees.
1> The syscon bank/group definition itself.
2> syscon register offsets and bits information to the drivers.
These are the 2 things which keep changing per each SOC.
There is no other way to pass this information to the drivers other than
passing them as part of their own device node and syscon node.
> and to the extent that it is sensible it feels like something
> which might be useful with any device using register maps, not just
> syscon.
If you think this is going to be useful for other drivers, Am happy to
move this out of syscon to regmap something like adding
of_regmap_field_claim/regmap_field_claim/regmap_field_read/regmap_field_write/regmap_field_release
functions.
so any exiting drivers can still use the old syscon API to get the
regmap instance.
Alternatively they can use the new regmap APIs directly.
regmap_field_claim to claim bits of the register
of_regmap_field_claim DT version of reg_map_field_claim.
regmap_field_write to write to bits of register
regmap_field_read to bit of the register.
regmap_field_release to release the bits of the register.
For DT version it might involve adding new member to struct
regmap_config to lookup regmap by phandle, so that regmap can get hold
of regmap instance from device tree phandle.
syscon driver already does this in syscon_regmap_lookup_by_phandle()
On the device tree side it will look like:
syscfg_sbc:syscon@fe600000{
compatible = "syscon";
reg = <0xfe600000 0x1000>;
};
node {
property-1 = <&syscfg_sbc 10 0 31>;
};
property has "phandle", register_offset, lsb, msb.
On the driver side, it can just use the API's like
field = of_regmap_field_claim(np, "property-1");
regmap_field_write(field, val);
regmap_field_release(field);
All this involves is very minimal code change in syscon and a new APIs
and DT awareness into regmap.
What do you think?
Thanks,
srini
> For example many MFDs have similar needs - essentially the
> system controllers are just a particular kind of MFD. To me that says
> that this should be outside syscon so other things can use it.
>
On Thu, May 09, 2013 at 12:58:01PM +0100, Srinivas KANDAGATLA wrote:
> Currently, we have two bits of information which come from device trees.
> 1> The syscon bank/group definition itself.
> 2> syscon register offsets and bits information to the drivers.
> These are the 2 things which keep changing per each SOC.
> There is no other way to pass this information to the drivers other than
> passing them as part of their own device node and syscon node.
Sure there is, for example the drivers could have this information
internally as part of knowing which device they're working with or they
could take advantage of some patterns in the register map to store some
higher level information that they use to configure.
> > and to the extent that it is sensible it feels like something
> > which might be useful with any device using register maps, not just
> > syscon.
> If you think this is going to be useful for other drivers, Am happy to
> move this out of syscon to regmap something like adding
> of_regmap_field_claim/regmap_field_claim/regmap_field_read/regmap_field_write/regmap_field_release
> functions.
> so any exiting drivers can still use the old syscon API to get the
> regmap instance.
> Alternatively they can use the new regmap APIs directly.
Well, I'd need to see the code to decide if it was sane but I do think
that if this is a good approach it's not syscon specific. Anything like
this needs to be independent of DT too since not all architectures use
DT.
On 09/05/13 14:26, Mark Brown wrote:
> On Thu, May 09, 2013 at 12:58:01PM +0100, Srinivas KANDAGATLA wrote:
>
>> Currently, we have two bits of information which come from device trees.
>> 1> The syscon bank/group definition itself.
>> 2> syscon register offsets and bits information to the drivers.
>
>> These are the 2 things which keep changing per each SOC.
>
>> There is no other way to pass this information to the drivers other than
>> passing them as part of their own device node and syscon node.
>
> Sure there is, for example the drivers could have this information
> internally as part of knowing which device they're working with or they
> could take advantage of some patterns in the register map to store some
> higher level information that they use to configure.
>
Some of layouts of the sysconf registers are totally changed for each SOC.
Looking at driver by driver maybe some drivers can take advantage of the
patterns, but Am not sure if this will be a sustainable solution for all
the drivers.
The big disadvantage of this approach is that
- Every driver has to be touched for new SOC,
- Secondly the drivers will end up having more of such information than
code over a time.
>>> and to the extent that it is sensible it feels like something
>>> which might be useful with any device using register maps, not just
>>> syscon.
>
>> If you think this is going to be useful for other drivers, Am happy to
>> move this out of syscon to regmap something like adding
>> of_regmap_field_claim/regmap_field_claim/regmap_field_read/regmap_field_write/regmap_field_release
>> functions.
>
>> so any exiting drivers can still use the old syscon API to get the
>> regmap instance.
>> Alternatively they can use the new regmap APIs directly.
>
> Well, I'd need to see the code to decide if it was sane but I do think
> that if this is a good approach it's not syscon specific. Anything like
> this needs to be independent of DT too since not all architectures use
> DT.
In the suggested approach, the API supports, both DT and non-DT style.
for DT style user can use.
of_regmap_field_claim -> regmap_field_read -> regmap_field_write-
->regmap_field_release
and for NON-DT user can use
regmap_field_claim -> regmap_field_read -> regmap_field_write-
->regmap_field_release
thanks,
srini
>
On Thu, May 09, 2013 at 03:00:16PM +0100, Srinivas KANDAGATLA wrote:
> Some of layouts of the sysconf registers are totally changed for each SOC.
> Looking at driver by driver maybe some drivers can take advantage of the
> patterns, but Am not sure if this will be a sustainable solution for all
> the drivers.
So what exactly is the driver doing then? If the register maps look
nothing like each other then what's the common functionality the driver
is providing?
On 09/05/13 15:40, Mark Brown wrote:
> So what exactly is the driver doing then? If the register maps look
> nothing like each other then what's the common functionality the driver
> is providing?
What I meant here is that, sysconf registers are reassigned per SOC, so
the sysconf register definitions change per SOC, not the register map of
the device itself.
The register map of the device itself still looks the same, however few
bits of the IP are wired up to the global sysconf registers, which is
what keeps changing, w.r.t sysconf number or bits of the sysconf itself.
Hi Marc,
I would like to explain you bit more about "ST System Configuration
registers".
The SOCs are assembled from existing IP blocks, which don't change very
often. However these blocks are assembled in different configurations to
meet the device requirements. So most IP blocks as well as having a bus
interface through which their own registers are accessible, will also
have a number of bristles(wires) which are signals that are going in and
out of the IP for configuration and status. To make these signals
accessible to software they are wired to "System Configuration Registers".
Drivers target the IP blocks, which don't change much. Where as the
mapping of IP specific bristles(wires) to "System Configuration
Registers" do change per each SOC, and therefore we do not want this
information to be part of the driver.
Having a System Configuration infrastructure gives much flexibility and
abstraction to drivers to configure them.
Given the comments on the patch, I am inclined not to use syscon for ST
specific "System Configuration Registers", alternatively I would like to
create a separate mfd system configuration driver specific to ST.
This is how we did it initially in the out-of-tree kernel.
http://git.stlinux.com/?p=stm/linux-stm.git;a=blob;f=drivers/stm/sysconf.c
Any suggestions?
thanks,
srini
On 09/05/13 10:51, Mark Brown wrote:
> On Wed, May 08, 2013 at 06:42:04PM +0100, Srinivas KANDAGATLA wrote:
>
> Fix your mailer to word wrap within paragraphs.
>
>> Ultimately the syscon_write use the regmap_update_bits, however we
>> really want is the flexibility in using/referring the syscon
>> registers/bits in both device-trees and non-device tree cases.
>
> So what you're looking for here is some way to offload discovery of
> register fields from the driver?
>
>> The reason for these APIs, is the extent of syscon usage is very high
>> in ST set-top-box parts.
>
>> Without these new APIs, its very difficult to pass this information to
>> the drivers.
>
> I'm not 100% convinced that putting all this information into DT is a
> good idea, and to the extent that it is sensible it feels like something
> which might be useful with any device using register maps, not just
> syscon. For example many MFDs have similar needs - essentially the
> system controllers are just a particular kind of MFD. To me that says
> that this should be outside syscon so other things can use it.
>
On Thursday 09 May 2013, Srinivas KANDAGATLA wrote:
> On 08/05/13 20:48, Arnd Bergmann wrote:
> I agree, my initial approach was having a dedicated driver specific to
> ST syscon, however syscon seems to do things very much similar to what
> we want, so I have integrated those 3 functions in syscon.
> Am happy to go back with my first approach of adding ST specific syscon
> driver if no one is actually going to benefit with such a change to
> syscon driver.
That would at least be less controversial.
> > Can you describe how your syscon registers are laid out?
> On STiH416 SOC we have 9 SYSCONF(aka System Configuration
> Registers)named banks/groups, each bank has its own memory map.
> Each sysconf bank has number of 32 bit registers which vary from bank to
> bank, like sysconf bank "sbc" has range from SYSTEM_CONFIG0 to
> SYSTEM_CONFIG999 where as sysconf bank "front" has range of
> SYSTEM_CONFIG1000 to SYSTEM_CONFIG1999 and so on.
>
> Each register is assigned with a unique SYCONF number, example:
> SYSTEM_CONFIG100, SYSTEM_CONFIG101 , .. and so on.
> Each sysconf contains bits of the IP configurations wired-up to the
> sysconf register bits.
Ok.
> As example:
>
> - Each pinctrl entry for set of 8 pins uses around 8-10 sysconfig
> register to control pinconf and pin functions.
> - IPs like Ethernet have few bit like Ethernet-Mode selection external
> or internal phyclk wired up to bits in sysconf registers,
> - Few clocks are controlled by sysconf registers.
> - Reset to IPs are wired up to bits of sysconf same registers.
> - ARM core soft reset is wired up to the sysconf registers...
> And most of the IPs have similar requirements ......
>
> Total layout of the sysconf changes per SOC, and the bit arrangements
> aswell, however the core IP(pinctrl, etherenet ...) and logic to drive
> those bits remains exactly same.
It sounds like you really need a driver with high-level interfaces
for the bits that change by each core and are needed by otherwise
identical drivers, like the Ethernet driver you mention.
I would not go as far as you did describing the individual bits in
the device node using these however. That driver can be layered on
top of the existing syscon driver, but hardcode the bits for each
SoC it knows of.
For drivers that are essentially just wrappers around sysconf,
I would make them one driver per SoC and use a low-level interface
but still hardcode the offsets in the driver instead of using DT
to find the registers.
The pinctrl and reset drivers are examples of this.
> In general the requirements of the sysconf support to the SOC/driver
> support is.
> 1> It should be able to read/write a sysconf register bits without
> having to "if" each SOC in the code. So that code is totally abstracted.
> Which is currently achieved by passing the information from the device
> trees and the driver just uses the property to get it.
The goal sounds fine, just the method is a bit more complex than necessary
here I think.
> 2> The infrastructure should protect the claimed registers from
> over-writing by other drivers. We do this by claim-read/write-release
> style API.
I don't understand this part. Is it about atomicity of accesses to
32-bit registers when you only want to change a bit? That is something
the regmap interface handles already.
If this is about drivers touching registers they should not touch
in the first place, I think it should not be needed at all, because
that would be a driver bug, and you can't really protect yourself
from broken drivers anyway.
> 3> The driver should be able to set a group of sysconf registers bits to
> a particular values before initialises the IP. I was thinking of doing
> this in a same way as pinctrl state.
That does not fit well with the model we use for other subsystems. If possible,
try to use the existing abstractions for clock, regulator, pinctrl, reset,
etc. and call generic interfaces from the driver. When that does not work,
create a high-level function call from your sysconf driver to do a particular
thing (e.g. stih_sysconf_ethernet_set_phy_mode()) rather than set up random
bits from the driver.
Arnd
Hi Arnd,
Thankyou for the comments.
On 17/05/13 15:36, Arnd Bergmann wrote:
> On Thursday 09 May 2013, Srinivas KANDAGATLA wrote:
>> On 08/05/13 20:48, Arnd Bergmann wrote:
>> I agree, my initial approach was having a dedicated driver specific to
>> ST syscon, however syscon seems to do things very much similar to what
>> we want, so I have integrated those 3 functions in syscon.
>> Am happy to go back with my first approach of adding ST specific syscon
>> driver if no one is actually going to benefit with such a change to
>> syscon driver.
>
> That would at least be less controversial.
yes I agree.
>
>> Total layout of the sysconf changes per SOC, and the bit arrangements
>> aswell, however the core IP(pinctrl, etherenet ...) and logic to drive
>> those bits remains exactly same.
>
> It sounds like you really need a driver with high-level interfaces
> for the bits that change by each core and are needed by otherwise
> identical drivers, like the Ethernet driver you mention.
>
> I would not go as far as you did describing the individual bits in
> the device node using these however. That driver can be layered on
> top of the existing syscon driver, but hardcode the bits for each
> SoC it knows of.
Some of the drivers like Ethernet already provide higher level
interfaces via callbacks. We did implement such a callbacks per each SOC
in non-DT case, and ended up having code duplicated for each SOC.
On the other hand using device trees to describe the HW
configuration(sysconfs) made more sense and got rid of SOC specific
callbacks.
>
> For drivers that are essentially just wrappers around sysconf,
> I would make them one driver per SoC and use a low-level interface
> but still hardcode the offsets in the driver instead of using DT
> to find the registers.
>
> The pinctrl and reset drivers are examples of this.
In pinctrl bindings case, I think we could do better job by replacing
the existing bindings of sysconfs for a group of banks with just two
integer offsets. This would mean that, we can still use the a common
driver across the SOCs.
And w.r.t to reset, we are planning on using sysconf based
reset-controller API sitting underneath the reset-controller subsystem.
Passing the information from device trees would be much clear and
flexible than adding new driver per/SOC.
>> 2> The infrastructure should protect the claimed registers from
>> over-writing by other drivers. We do this by claim-read/write-release
>> style API.
>
> I don't understand this part. Is it about atomicity of accesses to
> 32-bit registers when you only want to change a bit? That is something
> the regmap interface handles already.
I forget to mention a important point here, the protection is at bit
level for the sysconfs. Some of sysconfs have bits shared across IPs, so
protecting them while the IP is still using it is what we are trying to
achieve with sysconf-claim/release apis.
While it may be overkill, but In past it has found bugs and been helpful
with a meaning full debug output.
>> 3> The driver should be able to set a group of sysconf registers bits to
>> a particular values before initialises the IP. I was thinking of doing
>> this in a same way as pinctrl state.
>
> That does not fit well with the model we use for other subsystems. If possible,
> try to use the existing abstractions for clock, regulator, pinctrl, reset,
> etc. and call generic interfaces from the driver. When that does not work,
> create a high-level function call from your sysconf driver to do a particular
> thing (e.g. stih_sysconf_ethernet_set_phy_mode()) rather than set up random
> bits from the driver.
I agree going for a higher level api in this case makes sense.
Thanks,
srini
>
> Arnd
>
>
On Monday 20 May 2013, Srinivas KANDAGATLA wrote:
> On 17/05/13 15:36, Arnd Bergmann wrote:
>
> Some of the drivers like Ethernet already provide higher level
> interfaces via callbacks. We did implement such a callbacks per each SOC
> in non-DT case, and ended up having code duplicated for each SOC.
>
> On the other hand using device trees to describe the HW
> configuration(sysconfs) made more sense and got rid of SOC specific
> callbacks.
I'm sure it's possible to reduce the duplication without going all
the way to describing every bit in DT.
> > For drivers that are essentially just wrappers around sysconf,
> > I would make them one driver per SoC and use a low-level interface
> > but still hardcode the offsets in the driver instead of using DT
> > to find the registers.
> >
> > The pinctrl and reset drivers are examples of this.
>
> In pinctrl bindings case, I think we could do better job by replacing
> the existing bindings of sysconfs for a group of banks with just two
> integer offsets. This would mean that, we can still use the a common
> driver across the SOCs.
>
> And w.r.t to reset, we are planning on using sysconf based
> reset-controller API sitting underneath the reset-controller subsystem.
> Passing the information from device trees would be much clear and
> flexible than adding new driver per/SOC.
Ok
> >> 2> The infrastructure should protect the claimed registers from
> >> over-writing by other drivers. We do this by claim-read/write-release
> >> style API.
> >
> > I don't understand this part. Is it about atomicity of accesses to
> > 32-bit registers when you only want to change a bit? That is something
> > the regmap interface handles already.
>
> I forget to mention a important point here, the protection is at bit
> level for the sysconfs. Some of sysconfs have bits shared across IPs, so
> protecting them while the IP is still using it is what we are trying to
> achieve with sysconf-claim/release apis.
> While it may be overkill, but In past it has found bugs and been helpful
> with a meaning full debug output.
Can you give an example of something that is shared across multiple IPs?
Are those always done in the way that you have e.g. the ethernet pinctrl
in the same register as the usb pinctrl, or could you have an ethernet
pinctrl setting mixed together with the usb clock setting for instance?
If the hardware has been designed in a sensible way rather than just
grown by randomly adding bits in open spaces, I would assume that it's
possible to have high-level interfaces built around ranges of registers
rather than arbitrary subsets of registers.
Arnd
On 23/05/13 22:44, Arnd Bergmann wrote:
Thankyou Arnd for extending this discussion.
> On Monday 20 May 2013, Srinivas KANDAGATLA wrote:
>> On 17/05/13 15:36, Arnd Bergmann wrote:
>>
>> On the other hand using device trees to describe the HW
>> configuration(sysconfs) made more sense and got rid of SOC specific
>> callbacks.
>
> I'm sure it's possible to reduce the duplication without going all
> the way to describing every bit in DT.
Yes, I agree with you. I think just passing the offset information from
DT would enough in cases like Ethernet or USB where the sysconf
registers tend to be mostly standard. And the generic higher level
function should be able to do the rest.
>> I forget to mention a important point here, the protection is at bit
>> level for the sysconfs. Some of sysconfs have bits shared across IPs, so
>> protecting them while the IP is still using it is what we are trying to
>> achieve with sysconf-claim/release apis.
>> While it may be overkill, but In past it has found bugs and been helpful
>> with a meaning full debug output.
>
> Can you give an example of something that is shared across multiple IPs?
>
> Are those always done in the way that you have e.g. the ethernet pinctrl
> in the same register as the usb pinctrl, or could you have an ethernet
> pinctrl setting mixed together with the usb clock setting for instance?
>
> If the hardware has been designed in a sensible way rather than just
> grown by randomly adding bits in open spaces, I would assume that it's
> possible to have high-level interfaces built around ranges of registers
> rather than arbitrary subsets of registers.
We have decided to go with using higher level functions as you
suggested, hopefully based on regmap with some helper functions on top
of it.
System configuration registers in the past used to very much shared
across IPs, However with the new chips they seems be mostly dedicated
registers for specific IP configurations.
As we have no plans to support very old chips, So I think I can move
this out bit level information from device tree and add a higher level
functions to be accessed by the drivers.
As an example the below sysconf register for a STi7105 chip(Very old
SOC) looks like:
SYSTEM_CONFIG7 Comms/Ethernet config control register
[31:28] RESERVED
[27] ENMII:
1: MII mode 0: Reverse MII mode
[26:25] PHY_INTF_SELECT: Selects the type of Ethernet mode
00: MII mode (Default)
01: Reserved
1x: Reserved
[23] GLOBAL_POWER_DOWN:
1: Activate low power
[24] RESERVED
0: Normal mode
[22] DAA_CONFIG_CTRL: DAA configuration control
[21] RESERVED
[20] MAC_SPEED_SEL:
1: Indicates that the MAC is running at 100 Mbps speed
0: Indicates that the MAC is running at 10 Mbps speed
[19] RESERVED
[18] RMII_MODE:
1: RMII interface activated
0: MII interface activated
[17] MIIM_DIO_SELECT:
1: MIIM_DIO from external input, else from GMAC
0: All MII pads in input mode
[16] ETHERNET_INTERFACE_ON:
1: Ethernet on (pads enables controlled by MAC)
[15:13] RESERVED
[12] DAA_SERIAL_MODE: DAA serial interface mode select pin
[11] SC1_COND_VCC_ENABLE: Enables control of smart card VCC upon
bit in the PDES.
[10] SC_DETECT_VPP_POL:
1: Output pin SC_NOT_SETVPP is inverted of PDES_SC_SETVPP
0: Output pin SC_NOT_SETVPP is PDES_SC_SETVPP
[9] IRB_DATA_OUT_POL_OD: Selection of polarity of IRB output signal
routed as alternate function
...
...
Thanks,
srini
>
> Arnd
>
>