[PATCH 0/2] Add syscon name and #cells support

[PATCH 0/2] Add syscon name and #cells support

[Orson Zhai]

Our SoCs have a lot of glabal registers which is hard to be managed in 
current syscon structure.

Same register's offset is different in different SoCs. We used chip
config macro to manage them which prevents driver to be compiled in
all-in-one image.

After talking with Arnd and Rob at Linaro Connect 2017, I got the
idea to extend syscon with #cells support. And furthe, I added syscon
names support to help access multiple syscon nodes more easier.

These patches has been tested in our internal tree about 2 years.

They have no side effect to current syscon consumer.

Thanks,
Orson

--------------------
Orson Zhai (2):
  dt-bindings: Add syscon-names support
  mfd: syscon: Add syscon-names and phandle args support

 .../devicetree/bindings/mfd/syscon.txt        | 36 ++++++++++
 drivers/mfd/syscon.c                          | 65 +++++++++++++++++++
 include/linux/mfd/syscon.h                    | 22 +++++++
 3 files changed, 123 insertions(+)

-- 
2.18.0

[PATCH 1/2] dt-bindings: Add syscon-names support

[Orson Zhai]

Make life easier when syscon consumer want to access multiple syscon
nodes.
Add syscon-names and relative properties to help manage complicated
cases when accessing more one syscon node.

Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
---
 .../devicetree/bindings/mfd/syscon.txt        | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index 25d9e9c2fd53..ca7bc7608c15 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -17,6 +17,8 @@ Optional property:
 - reg-io-width: the size (in bytes) of the IO accesses that should be
   performed on the device.
 - hwlocks: reference to a phandle of a hardware spinlock provider node.
+- #syscon-cells: represents the number of args. Used when syscon-names
+  is going to be used. The value is vendor specific.
 
 Examples:
 gpr: iomuxc-gpr@20e0000 {
@@ -30,3 +32,37 @@ hwlock1: hwspinlock@40500000 {
 	reg = <0x40500000 0x1000>;
 	#hwlock-cells = <1>;
 };
+
+
+==Syscon names==
+
+Refer to syscon node by names with phandle args in syscon consumer node.
+
+Required properties:
+- syscons:	List of phandle and any number of args. Args is specific to
+		differnet vendor. For example: In Unisoc SoCs, the 1st arg
+		will be treated as register address offset and the 2nd is bit
+		mask as default.
+
+- syscon-names:	List of syscon node name strings sorted in the same
+		order as the syscons property.
+
+Examples:
+
+apb_regs: syscon@20008000 {
+	compatible = "sprd,apb-glb", "syscon";
+	#syscon-cells = <2>;
+	reg = <0x20008000 0x100>;
+};
+
+aon_regs: syscon@40008000 {
+	compatible = "sprd,aon-glb", "syscon";
+	#syscon-cells = <1>;
+	reg = <0x40008000 0x100>;
+};
+
+display@40500000 {
+	...
+	syscons = <&ap_apb_regs 0x4 0xf00>, <&aon_regs 0x8>;
+	syscon-names = "enable", "power";
+};
-- 
2.18.0

[PATCH 2/2] mfd: syscon: Add syscon-names and phandle args support

[Orson Zhai]

There are a lot of global registers used across multiple similar SoCs
from Unisoc. It is not easy to manage all of them very well by current
syscon device tree interfaces.

Add helper functions to get regmap and syscon args by syscon-names all
together.

This patch does not affect original syscon code and usage. It may help
other SoC vendors if they have the same trouble as well.

Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
---
 drivers/mfd/syscon.c       | 65 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/syscon.h | 22 +++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 660723276481..7ed1b2e4dba4 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -225,6 +225,71 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
 
+struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
+					const char *name)
+{
+	struct device_node *syscon_np;
+	struct of_phandle_args args;
+	struct regmap *regmap;
+	int index = 0;
+	int rc;
+
+	if (name)
+		index = of_property_match_string(np, "syscon-names", name);
+
+	if (index < 0)
+		return ERR_PTR(-EINVAL);
+
+	rc = of_parse_phandle_with_args(np, "syscons", "#syscon-cells", index,
+			&args);
+	if (rc)
+		return ERR_PTR(rc);
+
+	syscon_np = args.np;
+
+	if (!syscon_np)
+		return ERR_PTR(-ENODEV);
+
+	regmap = syscon_node_to_regmap(syscon_np);
+
+	of_node_put(syscon_np);
+
+	return regmap;
+}
+EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_name);
+
+int syscon_get_args_by_name(struct device_node *np,
+			const char *name,
+			int arg_count,
+			unsigned int *out_args)
+{
+	struct of_phandle_args args;
+	int index = 0;
+	int rc;
+
+	if (name)
+		index = of_property_match_string(np, "syscon-names", name);
+
+	if (index < 0)
+		return -EINVAL;
+
+	rc = of_parse_phandle_with_args(np, "syscons", "#syscon-cells", index,
+					&args);
+	if (rc)
+		return rc;
+
+	if (arg_count > args.args_count)
+		arg_count = args.args_count;
+
+	for (index = 0; index < arg_count; index++)
+		out_args[index] = args.args[index];
+
+	of_node_put(args.np);
+
+	return arg_count;
+}
+EXPORT_SYMBOL_GPL(syscon_get_args_by_name);
+
 static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 112dc66262cc..5584173cd5d4 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -23,6 +23,13 @@ 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);
+extern struct regmap *syscon_regmap_lookup_by_name(
+					struct device_node *np,
+					const char *name);
+extern int syscon_get_args_by_name(struct device_node *np,
+				const char *name,
+				int arg_count,
+				unsigned int *out_args);
 #else
 static inline struct regmap *device_node_to_regmap(struct device_node *np)
 {
@@ -45,6 +52,21 @@ static inline struct regmap *syscon_regmap_lookup_by_phandle(
 {
 	return ERR_PTR(-ENOTSUPP);
 }
+
+static inline struct regmap *syscon_regmap_lookup_by_name(
+					struct device_node *np,
+					const char *name)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+static int syscon_get_args_by_name(struct device_node *np,
+				const char *name,
+				int arg_count,
+				unsigned int *out_args)
+{
+	return -ENOTSUPP;
+}
 #endif
 
 #endif /* __LINUX_MFD_SYSCON_H__ */
-- 
2.18.0

Re: [PATCH 1/2] dt-bindings: Add syscon-names support

[Arnd Bergmann]

On Thu, Nov 14, 2019 at 12:48 PM Orson Zhai <orson.zhai@unisoc.com> wrote:
>
>
> Make life easier when syscon consumer want to access multiple syscon
> nodes.
> Add syscon-names and relative properties to help manage complicated
> cases when accessing more one syscon node.
>
> Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>

Hi Orson,

Can you explain why the number of cells in this binding is specific
to the syscon node rather than the node referencing it?

In most other bindings that follow the same scheme, the additional
arguments are interpreted by the subsystem that is being referenced,
but the syscon driver is just a simple driver with no subsystem and no
code to interpret those arguments.

The way would otherwise handle the example from your binding
would be with two separate properties in the display node, like

syscon-enable = <&ap_apb_regs 0x4 0xf00>;
syscon-power = <&aon_regs 0x8>;

in which case, the syscon driver does not need to know anything
about how it's being used, and the display driver is the one making
sense of the arguments according to its own binding.

I assume you have some good reason for introducing the other
approach, but I don't understand it from your submission.

       Arnd

Re: [PATCH 1/2] dt-bindings: Add syscon-names support

[Orson Zhai]

Hi Arnd,

On Fri, Nov 15, 2019 at 10:33:30AM +0100, Arnd Bergmann wrote:
> On Thu, Nov 14, 2019 at 12:48 PM Orson Zhai <orson.zhai@unisoc.com> wrote:
> >
> >
> > Make life easier when syscon consumer want to access multiple syscon
> > nodes.
> > Add syscon-names and relative properties to help manage complicated
> > cases when accessing more one syscon node.
> >
> > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
>
> Hi Orson,
>
> Can you explain why the number of cells in this binding is specific
> to the syscon node rather than the node referencing it?

The story is like this. I found there are too many global registers in
Unisoc(former Spreadtrum) chips. Dozens of offset with dozens of modules
were needed to be specified. So I thought the dts files would seem "horrible"
with a big chunk of syscon-xxx (say more than 20 lines)

I learned from reg-names way which might look clean to hold all these mess things.
But to implement this, the users need to konw the cell-size if we add arguments to syscon node.
I thought to add cell-size into every syscon consumer node is a duplicated work and
I wanted to take advantage of of_parse_phandle_with_args.
So the bindings were created then.


>
> In most other bindings that follow the same scheme, the additional
> arguments are interpreted by the subsystem that is being referenced,
> but the syscon driver is just a simple driver with no subsystem and no
> code to interpret those arguments.

You're correct. I've realized it is narraw to pass here.

>
> The way would otherwise handle the example from your binding
> would be with two separate properties in the display node, like
>
> syscon-enable = <&ap_apb_regs 0x4 0xf00>;
> syscon-power = <&aon_regs 0x8>;

This is an option for consumers all the time.
Acturally my patches are not going to replace this.
I'd like to provide another option to save people like desperate engineers in Spreadtrum :)

>
> in which case, the syscon driver does not need to know anything

Whould it be better if I add syscon-cells into consumer's node?
Then I could read the cell size and use "of_parse_phandle_with_fixed_args()" instead.
This will not involve syscon node itself at all.

Best Regards,
-Orson
> about how it's being used, and the display driver is the one making
> sense of the arguments according to its own binding.
>
> I assume you have some good reason for introducing the other
> approach, but I don't understand it from your submission.
>
>        Arnd
________________________________
 This email (including its attachments) is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. Unauthorized use, dissemination, distribution or copying of this email or the information herein or taking any action in reliance on the contents of this email or the information herein, by anyone other than the intended recipient, or an employee or agent responsible for delivering the message to the intended recipient, is strictly prohibited. If you are not the intended recipient, please do not read, copy, use or disclose any part of this e-mail to others. Please notify the sender immediately and permanently delete this e-mail and any attachments if you received it in error. Internet communications cannot be guaranteed to be timely, secure, error-free or virus-free. The sender does not accept liability for any errors or omissions.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。

Re: [PATCH 1/2] dt-bindings: Add syscon-names support

[Arnd Bergmann]

On Mon, Nov 18, 2019 at 9:42 AM Orson Zhai <orson.zhai@spreadtrum.com> wrote:
>
> Hi Arnd,
>
> On Fri, Nov 15, 2019 at 10:33:30AM +0100, Arnd Bergmann wrote:
> > On Thu, Nov 14, 2019 at 12:48 PM Orson Zhai <orson.zhai@unisoc.com> wrote:
> > >
> > >
> > > Make life easier when syscon consumer want to access multiple syscon
> > > nodes.
> > > Add syscon-names and relative properties to help manage complicated
> > > cases when accessing more one syscon node.
> > >
> > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> >
> > Hi Orson,
> >
> > Can you explain why the number of cells in this binding is specific
> > to the syscon node rather than the node referencing it?
>
> The story is like this. I found there are too many global registers in
> Unisoc(former Spreadtrum) chips. Dozens of offset with dozens of modules
> were needed to be specified. So I thought the dts files would seem "horrible"
> with a big chunk of syscon-xxx (say more than 20 lines)
>
> I learned from reg-names way which might look clean to hold all these mess things.
> But to implement this, the users need to konw the cell-size if we add arguments to syscon node.
> I thought to add cell-size into every syscon consumer node is a duplicated work and
> I wanted to take advantage of of_parse_phandle_with_args.
> So the bindings were created then.

Ok, that makes sense.

> > The way would otherwise handle the example from your binding
> > would be with two separate properties in the display node, like
> >
> > syscon-enable = <&ap_apb_regs 0x4 0xf00>;
> > syscon-power = <&aon_regs 0x8>;
>
> This is an option for consumers all the time.
> Acturally my patches are not going to replace this.
> I'd like to provide another option to save people like desperate engineers in Spreadtrum :)
>
> >
> > in which case, the syscon driver does not need to know anything
>
> Whould it be better if I add syscon-cells into consumer's node?

As I see it, there is no reason to put the syscon-cells property into any node,
as this is implied by the driver binding using the syscon reference.  I would
only use the #xyz-cells style property if there are multiple interpretations
that all make sense for the same binding.

> Then I could read the cell size and use "of_parse_phandle_with_fixed_args()" instead.
> This will not involve syscon node itself at all.

This sounds better to me, yes. I had not even remembered this function
exists, but I think this is a good idea.

I can also see a point in favor of adding more infrastructure around this,
possibly naming the entries in a syscon-names property as you suggested,
combining of_parse_phandle_with_fixed_args() with a name, or
combining with syscon_regmap_lookup_by_phandle() for convenience.

This should all be possible without adding complexity to the syscon
DT binding itself, and it would give more structure to the way it
is used by drivers.

       Arnd

Re: [PATCH 1/2] dt-bindings: Add syscon-names support

[Arnd Bergmann]

On Mon, Nov 18, 2019 at 9:42 AM Orson Zhai <orson.zhai@spreadtrum.com> wrote:
>
> Hi Arnd,
>
> On Fri, Nov 15, 2019 at 10:33:30AM +0100, Arnd Bergmann wrote:
> > On Thu, Nov 14, 2019 at 12:48 PM Orson Zhai <orson.zhai@unisoc.com> wrote:
> > >
> > >
> > > Make life easier when syscon consumer want to access multiple syscon
> > > nodes.
> > > Add syscon-names and relative properties to help manage complicated
> > > cases when accessing more one syscon node.
> > >
> > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> >
> > Hi Orson,
> >
> > Can you explain why the number of cells in this binding is specific
> > to the syscon node rather than the node referencing it?
>
> The story is like this. I found there are too many global registers in
> Unisoc(former Spreadtrum) chips. Dozens of offset with dozens of modules
> were needed to be specified. So I thought the dts files would seem "horrible"
> with a big chunk of syscon-xxx (say more than 20 lines)
>
> I learned from reg-names way which might look clean to hold all these mess things.
> But to implement this, the users need to konw the cell-size if we add arguments to syscon node.
> I thought to add cell-size into every syscon consumer node is a duplicated work and
> I wanted to take advantage of of_parse_phandle_with_args.
> So the bindings were created then.

Ok, that makes sense.

> > The way would otherwise handle the example from your binding
> > would be with two separate properties in the display node, like
> >
> > syscon-enable = <&ap_apb_regs 0x4 0xf00>;
> > syscon-power = <&aon_regs 0x8>;
>
> This is an option for consumers all the time.
> Acturally my patches are not going to replace this.
> I'd like to provide another option to save people like desperate engineers in Spreadtrum :)
>
> >
> > in which case, the syscon driver does not need to know anything
>
> Whould it be better if I add syscon-cells into consumer's node?

As I see it, there is no reason to put the syscon-cells property into any node,
as this is implied by the driver binding using the syscon reference.  I would
only use the #xyz-cells style property if there are multiple interpretations
that all make sense for the same binding.

> Then I could read the cell size and use "of_parse_phandle_with_fixed_args()" instead.
> This will not involve syscon node itself at all.

This sounds better to me, yes. I had not even remembered this function
exists, but I think this is a good idea.

I can also see a point in favor of adding more infrastructure around this,
possibly naming the entries in a syscon-names property as you suggested,
combining of_parse_phandle_with_fixed_args() with a name, or
combining with syscon_regmap_lookup_by_phandle() for convenience.

This should all be possible without adding complexity to the syscon
DT binding itself, and it would give more structure to the way it
is used by drivers.

       Arnd

Re: [PATCH 1/2] dt-bindings: Add syscon-names support

[Orson Zhai]

Hi Arnd,

On Tue, Nov 19, 2019 at 03:46:09PM +0100, Arnd Bergmann wrote:
> On Mon, Nov 18, 2019 at 9:42 AM Orson Zhai <orson.zhai@spreadtrum.com> wrote:
> >
> > Hi Arnd,
> >
> > On Fri, Nov 15, 2019 at 10:33:30AM +0100, Arnd Bergmann wrote:
> > > On Thu, Nov 14, 2019 at 12:48 PM Orson Zhai <orson.zhai@unisoc.com> wrote:
> > > >
> > > >
> > > > Make life easier when syscon consumer want to access multiple syscon
> > > > nodes.
> > > > Add syscon-names and relative properties to help manage complicated
> > > > cases when accessing more one syscon node.
> > > >
> > > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com>
> > >
> > > Hi Orson,
> > >
> > > Can you explain why the number of cells in this binding is specific
> > > to the syscon node rather than the node referencing it?
> >
> > The story is like this. I found there are too many global registers in
> > Unisoc(former Spreadtrum) chips. Dozens of offset with dozens of modules
> > were needed to be specified. So I thought the dts files would seem "horrible"
> > with a big chunk of syscon-xxx (say more than 20 lines)
> >
> > I learned from reg-names way which might look clean to hold all these mess things.
> > But to implement this, the users need to konw the cell-size if we add arguments to syscon node.
> > I thought to add cell-size into every syscon consumer node is a duplicated work and
> > I wanted to take advantage of of_parse_phandle_with_args.
> > So the bindings were created then.
>
> Ok, that makes sense.
>
> > > The way would otherwise handle the example from your binding
> > > would be with two separate properties in the display node, like
> > >
> > > syscon-enable = <&ap_apb_regs 0x4 0xf00>;
> > > syscon-power = <&aon_regs 0x8>;
> >
> > This is an option for consumers all the time.
> > Acturally my patches are not going to replace this.
> > I'd like to provide another option to save people like desperate engineers in Spreadtrum :)
> >
> > >
> > > in which case, the syscon driver does not need to know anything
> >
> > Whould it be better if I add syscon-cells into consumer's node?
>
> As I see it, there is no reason to put the syscon-cells property into any node,
> as this is implied by the driver binding using the syscon reference.  I would
> only use the #xyz-cells style property if there are multiple interpretations
> that all make sense for the same binding.
>
> > Then I could read the cell size and use "of_parse_phandle_with_fixed_args()" instead.
> > This will not involve syscon node itself at all.
>
> This sounds better to me, yes. I had not even remembered this function
> exists, but I think this is a good idea.
>
> I can also see a point in favor of adding more infrastructure around this,
> possibly naming the entries in a syscon-names property as you suggested,
> combining of_parse_phandle_with_fixed_args() with a name, or
> combining with syscon_regmap_lookup_by_phandle() for convenience.
>
> This should all be possible without adding complexity to the syscon
> DT binding itself, and it would give more structure to the way it
> is used by drivers.

V2 has been sent out per as you suggested.

Free free to review please.

Best Regards,
-Orson
>
>        Arnd
________________________________
 This email (including its attachments) is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. Unauthorized use, dissemination, distribution or copying of this email or the information herein or taking any action in reliance on the contents of this email or the information herein, by anyone other than the intended recipient, or an employee or agent responsible for delivering the message to the intended recipient, is strictly prohibited. If you are not the intended recipient, please do not read, copy, use or disclose any part of this e-mail to others. Please notify the sender immediately and permanently delete this e-mail and any attachments if you received it in error. Internet communications cannot be guaranteed to be timely, secure, error-free or virus-free. The sender does not accept liability for any errors or omissions.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。