Hi,
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 from being compiled
in all-in-one image.
So I add syscons, syscon-names and an optional #vendor-cells property
as syscon consumer node's bindings. And implement coresponding helper
functions.
They have no side effect to current syscon consumer.
Thanks,
Orson
Changes in V2:
As per suggestion from Arnd:
* Remove #syscon-cells from syscon node.
* Add "#vendor-cells" into consumer node not affecting referred syscon
itself.
* Change helper funcions parameter accordingly.
-----
Orson Zhai (2):
dt-bindings: syscon: Add syscon-names to refer to syscon easily
mfd: syscon: Find syscon by names with arguments support
.../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++
drivers/mfd/syscon.c | 75 +++++++++++++++++++
include/linux/mfd/syscon.h | 26 +++++++
3 files changed, 144 insertions(+)
--
2.18.0
________________________________
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.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。
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 helper functions.
Add helper functions to get regmap and arguments 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 <[email protected]>
---
drivers/mfd/syscon.c | 75 ++++++++++++++++++++++++++++++++++++++
include/linux/mfd/syscon.h | 26 +++++++++++++
2 files changed, 101 insertions(+)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 660723276481..e818decc7bf2 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -225,6 +225,81 @@ 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 *list_name,
+ const char *cell_name)
+{
+ struct device_node *syscon_np;
+ struct of_phandle_args args;
+ struct regmap *regmap;
+ unsigned int index = 0, cell_count = 0;
+ int rc;
+
+ if (list_name)
+ index = of_property_match_string(np, "syscon-names", list_name);
+
+ if (index < 0)
+ return ERR_PTR(-EINVAL);
+
+
+ if (cell_name && of_property_read_u32(np, cell_name, &cell_count))
+ return ERR_PTR(-EINVAL);
+
+ rc = of_parse_phandle_with_fixed_args(np, "syscons", cell_count,
+ 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 *list_name,
+ const char *cell_name,
+ int arg_count,
+ unsigned int *out_args)
+{
+ struct of_phandle_args args;
+ unsigned int index = 0, cell_count = 0;
+
+ int rc;
+
+ if (list_name)
+ index = of_property_match_string(np, "syscon-names", list_name);
+
+ if (index < 0)
+ return -EINVAL;
+
+ if (cell_name && of_property_read_u32(np, cell_name, &cell_count))
+ return -EINVAL;
+
+ rc = of_parse_phandle_with_fixed_args(np, "syscons", cell_count,
+ 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..96bdaf3e63fd 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -23,6 +23,15 @@ 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 *list_name,
+ const char *cell_name);
+extern int syscon_get_args_by_name(struct device_node *np,
+ const char *list_name,
+ const char *cell_name,
+ int arg_count,
+ unsigned int *out_args);
#else
static inline struct regmap *device_node_to_regmap(struct device_node *np)
{
@@ -45,6 +54,23 @@ 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 *list_name,
+ const char *cell_name)
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
+static int syscon_get_args_by_name(struct device_node *np,
+ const char *list_name,
+ const char *cell_name,
+ int arg_count,
+ unsigned int *out_args)
+{
+ return -ENOTSUPP;
+}
#endif
#endif /* __LINUX_MFD_SYSCON_H__ */
--
2.18.0
________________________________
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.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。
Make life easier when syscon consumer want to access multiple syscon
nodes with dozens of items.
Add syscon-names and relative properties to help to manage different
cases when accessing more than one syscon node even with arguments.
Signed-off-by: Orson Zhai <[email protected]>
---
.../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index 25d9e9c2fd53..4c7bdb74bb0a 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 {
reg = <0x40500000 0x1000>;
#hwlock-cells = <1>;
};
+
+
+
+==Syscon Name==
+
+Syscon name is a helper to be used in consumer nodes who refer to system
+controller node. It provides a way to refer to syscon node by names with
+phandle args in syscon consumer node. It will help people who have a lot
+of syscon references to be managed. It is not a must feature and has no
+effect to syscon node itself at all.
+
+Required properties:
+- syscons: List of phandles and any number of arguments if needed. Arguments
+ is specific to differnet vendors and its usage should be described at each
+ vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated
+ as register address offset and the 2nd is bit mask.
+
+- syscon-names: List of syscon node name strings sorted in the same order as
+ what it represents in property syscons.
+
+Optional property:
+- #.*-cells: Represents the number of arguments in single phandle in syscons
+ list. ".*" is vendor specific. If this property is not set, default value
+ will be 0.
+
+Examples:
+
+apb_regs: syscon@20008000 {
+ compatible = "sprd,apb-glb", "syscon";
+ reg = <0x20008000 0x100>;
+};
+
+aon_regs: syscon@40008000 {
+ compatible = "sprd,aon-glb", "syscon";
+ reg = <0x40008000 0x100>;
+};
+
+display@40500000 {
+ ...
+ #syscon-disp-cells = <2>;
+ syscons = <&ap_apb_regs 0x4 0xf00>, <&aon_regs 0x8 0x7>;
+ syscon-names = "enable", "power";
+};
--
2.18.0
________________________________
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.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。
On Wed, Nov 20, 2019 at 4:44 PM Orson Zhai <[email protected]> wrote:
>
> +
> +Optional property:
> +- #.*-cells: Represents the number of arguments in single phandle in syscons
> + list. ".*" is vendor specific. If this property is not set, default value
> + will be 0.
> +
> +Examples:
> +
> +apb_regs: syscon@20008000 {
> + compatible = "sprd,apb-glb", "syscon";
> + reg = <0x20008000 0x100>;
> +};
> +
> +aon_regs: syscon@40008000 {
> + compatible = "sprd,aon-glb", "syscon";
> + reg = <0x40008000 0x100>;
> +};
> +
> +display@40500000 {
> + ...
> + #syscon-disp-cells = <2>;
> + syscons = <&ap_apb_regs 0x4 0xf00>, <&aon_regs 0x8 0x7>;
> + syscon-names = "enable", "power";
> +};
Hi Orson,
With the changes from the syscon nodes removed, it looks better
already, but I'd also like to not see the #.*-cells mentioned at
all, as there is not really a way to parse this automatically, or
make sense of the data without already knowing how many cells
there are.
Arnd
On Wed, Nov 20, 2019 at 4:44 PM Orson Zhai <[email protected]> wrote:
>
> 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 helper functions.
>
> Add helper functions to get regmap and arguments 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 <[email protected]>
> ---
> drivers/mfd/syscon.c | 75 ++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/syscon.h | 26 +++++++++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 660723276481..e818decc7bf2 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -225,6 +225,81 @@ 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 *list_name,
> + const char *cell_name)
> +{
According to the binding change I suggested, this would not take a 'cell_name'
argument, but instead a an arg_count.
> +
> +int syscon_get_args_by_name(struct device_node *np,
> + const char *list_name,
> + const char *cell_name,
> + int arg_count,
> + unsigned int *out_args)
> +{
and I think this could be combined with it, like
struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
const char *name, int
arg_count, __u32 *out_args)
Arnd
On Thu, Nov 21, 2019 at 11:06 PM Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Nov 20, 2019 at 4:44 PM Orson Zhai <[email protected]> wrote:
> >
> > 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 helper functions.
> >
> > Add helper functions to get regmap and arguments 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 <[email protected]>
> > ---
> > drivers/mfd/syscon.c | 75 ++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/syscon.h | 26 +++++++++++++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 660723276481..e818decc7bf2 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -225,6 +225,81 @@ 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 *list_name,
> > + const char *cell_name)
> > +{
>
> According to the binding change I suggested, this would not take a 'cell_name'
> argument, but instead a an arg_count.
Got it. This is much convenient for caller.
>
> > +
> > +int syscon_get_args_by_name(struct device_node *np,
> > + const char *list_name,
> > + const char *cell_name,
> > + int arg_count,
> > + unsigned int *out_args)
> > +{
>
> and I think this could be combined with it, like
>
> struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
> const char *name, int
> arg_count, __u32 *out_args)
Ok, I'll send V3 next week.
Best,
-Orson
>
> Arnd
On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote:
> Make life easier when syscon consumer want to access multiple syscon
> nodes with dozens of items.
> Add syscon-names and relative properties to help to manage different
> cases when accessing more than one syscon node even with arguments.
>
> Signed-off-by: Orson Zhai <[email protected]>
> ---
> .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> index 25d9e9c2fd53..4c7bdb74bb0a 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 {
> reg = <0x40500000 0x1000>;
> #hwlock-cells = <1>;
> };
> +
> +
> +
> +==Syscon Name==
> +
> +Syscon name is a helper to be used in consumer nodes who refer to system
> +controller node. It provides a way to refer to syscon node by names with
> +phandle args in syscon consumer node. It will help people who have a lot
> +of syscon references to be managed. It is not a must feature and has no
> +effect to syscon node itself at all.
> +
> +Required properties:
> +- syscons: List of phandles and any number of arguments if needed. Arguments
> + is specific to differnet vendors and its usage should be described at each
> + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated
> + as register address offset and the 2nd is bit mask.
> +
> +- syscon-names: List of syscon node name strings sorted in the same order as
> + what it represents in property syscons.
> +
> +Optional property:
> +- #.*-cells: Represents the number of arguments in single phandle in syscons
> + list. ".*" is vendor specific. If this property is not set, default value
> + will be 0.
This breaks the normal pattern of how '#.*-cells' works. While Arnd
suggests removing it, I don't think that works well either with having a
generic 'syscons' property. That means every syscon in a system has to
have the same number of cells.
I don't really want to see syscon binding expanded. Really, I'd like
'syscon' to go away. It's nothing more than a flag to create a regmap.
I think it is better to keep the property names specific to exactly what
the functionality is for each syscon phandle rather than a generic
binding.
What are the eamples of where you want to use this? Keep in mind that
this sort of connection should *only* be used for things that have no
other binding and kernel subsystem.
Rob
On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <[email protected]> wrote:
> On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote:
> > Make life easier when syscon consumer want to access multiple syscon
> > nodes with dozens of items.
> > Add syscon-names and relative properties to help to manage different
> > cases when accessing more than one syscon node even with arguments.
> >
> > Signed-off-by: Orson Zhai <[email protected]>
> > ---
> > .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > index 25d9e9c2fd53..4c7bdb74bb0a 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 {
> > reg = <0x40500000 0x1000>;
> > #hwlock-cells = <1>;
> > };
> > +
> > +
> > +
> > +==Syscon Name==
> > +
> > +Syscon name is a helper to be used in consumer nodes who refer to system
> > +controller node. It provides a way to refer to syscon node by names with
> > +phandle args in syscon consumer node. It will help people who have a lot
> > +of syscon references to be managed. It is not a must feature and has no
> > +effect to syscon node itself at all.
> > +
> > +Required properties:
> > +- syscons: List of phandles and any number of arguments if needed. Arguments
> > + is specific to differnet vendors and its usage should be described at each
> > + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated
> > + as register address offset and the 2nd is bit mask.
> > +
> > +- syscon-names: List of syscon node name strings sorted in the same order as
> > + what it represents in property syscons.
> > +
> > +Optional property:
> > +- #.*-cells: Represents the number of arguments in single phandle in syscons
> > + list. ".*" is vendor specific. If this property is not set, default value
> > + will be 0.
>
> This breaks the normal pattern of how '#.*-cells' works. While Arnd
> suggests removing it, I don't think that works well either with having a
> generic 'syscons' property. That means every syscon in a system has to
> have the same number of cells.
>
> I don't really want to see syscon binding expanded. Really, I'd like
> 'syscon' to go away. It's nothing more than a flag to create a regmap.
Not expanding the syscon binding is the point about not having a #*-cells:
In the examples that Orson listed, the cell count would always be
specific to the user of the syscon regmap, and not interpreted by the
syscon itself.
> I think it is better to keep the property names specific to exactly what
> the functionality is for each syscon phandle rather than a generic
> binding.
>
> What are the eamples of where you want to use this?
I think generally speaking this would be useful for random registers that
logically belong to one device but are grouped with other unrelated
registers in a syscon, and that are in a different register offset for
each chip that has them. Using named properties instead of a list
of phandle/arg tuples with names is clearly a simpler alternative
and more like we do it today, but I can also see some API simplification
with Orson's patch without the binding getting out of hand.
> Keep in mind that
> this sort of connection should *only* be used for things that have no
> other binding and kernel subsystem.
+1
Arnd
On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <[email protected]> wrote:
> > On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote:
> > > Make life easier when syscon consumer want to access multiple syscon
> > > nodes with dozens of items.
> > > Add syscon-names and relative properties to help to manage different
> > > cases when accessing more than one syscon node even with arguments.
> > >
> > > Signed-off-by: Orson Zhai <[email protected]>
> > > ---
> > > .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++
> > > 1 file changed, 43 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > index 25d9e9c2fd53..4c7bdb74bb0a 100644
> > > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 {
> > > reg = <0x40500000 0x1000>;
> > > #hwlock-cells = <1>;
> > > };
> > > +
> > > +
> > > +
> > > +==Syscon Name==
> > > +
> > > +Syscon name is a helper to be used in consumer nodes who refer to system
> > > +controller node. It provides a way to refer to syscon node by names with
> > > +phandle args in syscon consumer node. It will help people who have a lot
> > > +of syscon references to be managed. It is not a must feature and has no
> > > +effect to syscon node itself at all.
> > > +
> > > +Required properties:
> > > +- syscons: List of phandles and any number of arguments if needed. Arguments
> > > + is specific to differnet vendors and its usage should be described at each
> > > + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated
> > > + as register address offset and the 2nd is bit mask.
> > > +
> > > +- syscon-names: List of syscon node name strings sorted in the same order as
> > > + what it represents in property syscons.
> > > +
> > > +Optional property:
> > > +- #.*-cells: Represents the number of arguments in single phandle in syscons
> > > + list. ".*" is vendor specific. If this property is not set, default value
> > > + will be 0.
> >
> > This breaks the normal pattern of how '#.*-cells' works. While Arnd
> > suggests removing it, I don't think that works well either with having a
> > generic 'syscons' property. That means every syscon in a system has to
> > have the same number of cells.
> >
> > I don't really want to see syscon binding expanded. Really, I'd like
> > 'syscon' to go away. It's nothing more than a flag to create a regmap.
>
> Not expanding the syscon binding is the point about not having a #*-cells:
> In the examples that Orson listed, the cell count would always be
> specific to the user of the syscon regmap, and not interpreted by the
> syscon itself.
>
> > I think it is better to keep the property names specific to exactly what
> > the functionality is for each syscon phandle rather than a generic
> > binding.
> >
> > What are the eamples of where you want to use this?
>
> I think generally speaking this would be useful for random registers that
> logically belong to one device but are grouped with other unrelated
> registers in a syscon, and that are in a different register offset for
> each chip that has them. Using named properties instead of a list
> of phandle/arg tuples with names is clearly a simpler alternative
> and more like we do it today, but I can also see some API simplification
> with Orson's patch without the binding getting out of hand.
I understand when a phandle to a syscon is used. That's nothing new.
What's special about Unisoc SoC that needs something new/different?
I saw there's a large number of syscons, but I don't understand what's
in them.
If the API is this:
struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
const char *name,
int arg_count, __u32 *out_args)
How is 'name' being an entry in syscon-names simpler than just being the
property name? The implementation for the latter would certainly be
simpler.
It also makes the property unparseable without knowledge outside of the
DT (i.e. in the driver). I suppose if the number of cells for each entry
is fixed, we could count the number of syscon-names entries to figure
out the stride. But then if one entry needs a lot of cells, then they
all have to have padding cells.
Rob
On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <[email protected]> wrote:
> On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <[email protected]> wrote:
> > I think generally speaking this would be useful for random registers that
> > logically belong to one device but are grouped with other unrelated
> > registers in a syscon, and that are in a different register offset for
> > each chip that has them. Using named properties instead of a list
> > of phandle/arg tuples with names is clearly a simpler alternative
> > and more like we do it today, but I can also see some API simplification
> > with Orson's patch without the binding getting out of hand.
>
> I understand when a phandle to a syscon is used. That's nothing new.
> What's special about Unisoc SoC that needs something new/different?
> I saw there's a large number of syscons, but I don't understand what's
> in them.
>
> If the API is this:
>
> struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
> const char *name,
> int arg_count, __u32 *out_args)
>
> How is 'name' being an entry in syscon-names simpler than just being the
> property name? The implementation for the latter would certainly be
> simpler.
>
> It also makes the property unparseable without knowledge outside of the
> DT (i.e. in the driver). I suppose if the number of cells for each entry
> is fixed, we could count the number of syscon-names entries to figure
> out the stride. But then if one entry needs a lot of cells, then they
> all have to have padding cells.
Good point. The syscon_regmap_lookup_by_name() interface would
work just as well when passing a property name compared to
a name listed in another property, and this would still be more in
line with what we do on other SoCs.
The only advantage I can see in having a list of phandle/arg tuples
rather than a set of properties is that it is a slightly more compact
representation in source form, but otherwise they should be equivalent
and agree about this being harder to parse in an automated way.
Orson, do you see any other reason for the combined property?
If not, could you respin the series once more with
syscon_regmap_lookup_by_name() replaced by something like:?
struct regmap *
syscon_regmap_lookup_args_by_phandle(struct device_node *np,
const char *property,
int arg_count, __u32 *out_args)
Arnd
Hi Arnd & Rob,
On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote:
> On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <[email protected]> wrote:
> > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <[email protected]> wrote:
>
> > > I think generally speaking this would be useful for random registers that
> > > logically belong to one device but are grouped with other unrelated
> > > registers in a syscon, and that are in a different register offset for
> > > each chip that has them. Using named properties instead of a list
> > > of phandle/arg tuples with names is clearly a simpler alternative
> > > and more like we do it today, but I can also see some API simplification
> > > with Orson's patch without the binding getting out of hand.
> >
> > I understand when a phandle to a syscon is used. That's nothing new.
> > What's special about Unisoc SoC that needs something new/different?
> > I saw there's a large number of syscons, but I don't understand what's
> > in them.
> >
> > If the API is this:
> >
> > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
> > const char *name,
> > int arg_count, __u32 *out_args)
> >
> > How is 'name' being an entry in syscon-names simpler than just being the
> > property name? The implementation for the latter would certainly be
> > simpler.
> >
> > It also makes the property unparseable without knowledge outside of the
> > DT (i.e. in the driver). I suppose if the number of cells for each entry
> > is fixed, we could count the number of syscon-names entries to figure
> > out the stride. But then if one entry needs a lot of cells, then they
> > all have to have padding cells.
>
> Good point. The syscon_regmap_lookup_by_name() interface would
> work just as well when passing a property name compared to
> a name listed in another property, and this would still be more in
> line with what we do on other SoCs.
>
udx710-modem.dtsi:69: syscons = <&pmu_apb_regs 0x18 0x2000000>,
udx710-modem.dtsi-70- <&pmu_apb_regs 0x544 0x1>,
udx710-modem.dtsi-71- <&aon_apb_regs 0x218 0x7e00>,
udx710-modem.dtsi-72- <&pmu_apb_regs 0xb0 0x20000>,
udx710-modem.dtsi-73- <&pmu_apb_regs 0xff 0x100>;
udx710-modem.dtsi:74: syscon-names = "shutdown", "deepsleep", "corereset",
udx710-modem.dtsi-75- "sysreset", "getstatus";
ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
ud710.dtsi:1272: syscon-names = "sd_detect_pol",
ud710.dtsi-1273- "sd_hotplug_protect_en",
ud710.dtsi-1274- "sd_hotplug_debounce_en",
ud710.dtsi-1275- "sd_hotplug_debounce_cn";
> The only advantage I can see in having a list of phandle/arg tuples
> rather than a set of properties is that it is a slightly more compact
> representation in source form, but otherwise they should be equivalent
Yes, I agree.
They are equivalent.
But sprd SoCs have too many registers and the representation might matter.
Here's some real code from local,
orca.dtsi:1276: syscons = <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_SYS_CFG MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >,
orca.dtsi-1277- <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>,
orca.dtsi-1278- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>,
orca.dtsi-1279- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>,
orca.dtsi-1280- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>,
orca.dtsi-1281- <&pmu_apb_regs REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>,
orca.dtsi-1282- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>,
orca.dtsi-1283- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>,
orca.dtsi-1284- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>,
--
orca.dtsi:1288: syscon-names = "sysshutdown", "coreshutdown", "deepsleep", "corereset",
orca.dtsi-1289- "sysreset", "reset_sel", "sysstatus", "corestatus", "sleepstatus",
orca.dtsi-1290- "bootprotect", "bootvector", "bootaddress_sel";
ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
ud710.dtsi:1272: syscon-names = "sd_detect_pol",
ud710.dtsi-1273- "sd_hotplug_protect_en",
ud710.dtsi-1274- "sd_hotplug_debounce_en",
ud710.dtsi-1275- "sd_hotplug_debounce_cn";
Compare to following,
sd_hotplug_protect_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>;
sd_hotplug_debounce_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>;
sd_hotplug_debounce_cn-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
.....
For me, my choice would be the former.
It looks more clear.
> and agree about this being harder to parse in an automated way.
>
> Orson, do you see any other reason for the combined property?
No other reason.
> If not, could you respin the series once more with
> syscon_regmap_lookup_by_name() replaced by something like:?
>
> struct regmap *
> syscon_regmap_lookup_args_by_phandle(struct device_node *np,
> const char *property,
> int arg_count, __u32 *out_args)
I like this idea. syscon_regmap_lookup_by_phandle_args() would be better?
May I impelement them both?
But I will be OK if no one here except me likes to expand name list.
Best,
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.
本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。
On Thu, Dec 05, 2019 at 08:55:39PM +0800, Orson Zhai wrote:
> Hi Arnd & Rob,
>
> On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote:
> > On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <[email protected]> wrote:
> > > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> > > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <[email protected]> wrote:
> >
> > > > I think generally speaking this would be useful for random registers that
> > > > logically belong to one device but are grouped with other unrelated
> > > > registers in a syscon, and that are in a different register offset for
> > > > each chip that has them. Using named properties instead of a list
> > > > of phandle/arg tuples with names is clearly a simpler alternative
> > > > and more like we do it today, but I can also see some API simplification
> > > > with Orson's patch without the binding getting out of hand.
> > >
> > > I understand when a phandle to a syscon is used. That's nothing new.
> > > What's special about Unisoc SoC that needs something new/different?
> > > I saw there's a large number of syscons, but I don't understand what's
> > > in them.
> > >
> > > If the API is this:
> > >
> > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
> > > const char *name,
> > > int arg_count, __u32 *out_args)
> > >
> > > How is 'name' being an entry in syscon-names simpler than just being the
> > > property name? The implementation for the latter would certainly be
> > > simpler.
> > >
> > > It also makes the property unparseable without knowledge outside of the
> > > DT (i.e. in the driver). I suppose if the number of cells for each entry
> > > is fixed, we could count the number of syscon-names entries to figure
> > > out the stride. But then if one entry needs a lot of cells, then they
> > > all have to have padding cells.
> >
> > Good point. The syscon_regmap_lookup_by_name() interface would
> > work just as well when passing a property name compared to
> > a name listed in another property, and this would still be more in
> > line with what we do on other SoCs.
> >
>
> udx710-modem.dtsi:69: syscons = <&pmu_apb_regs 0x18 0x2000000>,
> udx710-modem.dtsi-70- <&pmu_apb_regs 0x544 0x1>,
> udx710-modem.dtsi-71- <&aon_apb_regs 0x218 0x7e00>,
> udx710-modem.dtsi-72- <&pmu_apb_regs 0xb0 0x20000>,
> udx710-modem.dtsi-73- <&pmu_apb_regs 0xff 0x100>;
> udx710-modem.dtsi:74: syscon-names = "shutdown", "deepsleep", "corereset",
> udx710-modem.dtsi-75- "sysreset", "getstatus";
Reset at least has a standard binding.
> ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
> ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
> ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
> ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> ud710.dtsi:1272: syscon-names = "sd_detect_pol",
> ud710.dtsi-1273- "sd_hotplug_protect_en",
> ud710.dtsi-1274- "sd_hotplug_debounce_en",
> ud710.dtsi-1275- "sd_hotplug_debounce_cn";
This looks to me like it should be a single phandle. How many different
register layouts across how many SoCs do you need to support?
> > The only advantage I can see in having a list of phandle/arg tuples
> > rather than a set of properties is that it is a slightly more compact
> > representation in source form, but otherwise they should be equivalent
>
> Yes, I agree.
> They are equivalent.
>
> But sprd SoCs have too many registers and the representation might matter.
> Here's some real code from local,
>
> orca.dtsi:1276: syscons = <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_SYS_CFG MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >,
> orca.dtsi-1277- <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>,
> orca.dtsi-1278- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>,
> orca.dtsi-1279- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>,
> orca.dtsi-1280- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>,
> orca.dtsi-1281- <&pmu_apb_regs REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>,
> orca.dtsi-1282- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>,
> orca.dtsi-1283- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>,
> orca.dtsi-1284- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>,
> --
> orca.dtsi:1288: syscon-names = "sysshutdown", "coreshutdown", "deepsleep", "corereset",
> orca.dtsi-1289- "sysreset", "reset_sel", "sysstatus", "corestatus", "sleepstatus",
> orca.dtsi-1290- "bootprotect", "bootvector", "bootaddress_sel";
Again, reset has standard binding.
Also consider if you really need to access all of these vs. assuming a
fixed mode that the firmware/bootloader sets up.
> ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
> ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
> ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
> ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> ud710.dtsi:1272: syscon-names = "sd_detect_pol",
> ud710.dtsi-1273- "sd_hotplug_protect_en",
> ud710.dtsi-1274- "sd_hotplug_debounce_en",
> ud710.dtsi-1275- "sd_hotplug_debounce_cn";
>
> Compare to following,
>
> sd_hotplug_protect_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>;
> sd_hotplug_debounce_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>;
> sd_hotplug_debounce_cn-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> .....
>
> For me, my choice would be the former.
> It looks more clear.
>
> > and agree about this being harder to parse in an automated way.
> >
> > Orson, do you see any other reason for the combined property?
> No other reason.
>
> > If not, could you respin the series once more with
> > syscon_regmap_lookup_by_name() replaced by something like:?
> >
> > struct regmap *
> > syscon_regmap_lookup_args_by_phandle(struct device_node *np,
> > const char *property,
> > int arg_count, __u32 *out_args)
>
> I like this idea. syscon_regmap_lookup_by_phandle_args() would be better?
>
> May I impelement them both?
No.
Rob
Sorry! Re-send as plain-text mode.
On Thu, Dec 5, 2019 at 11:13 PM Rob Herring <[email protected]> wrote:
>
> On Thu, Dec 05, 2019 at 08:55:39PM +0800, Orson Zhai wrote:
> > Hi Arnd & Rob,
> >
> > On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote:
> > > On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <[email protected]> wrote:
> > > > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> > > > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <[email protected]> wrote:
> > >
> > > > > I think generally speaking this would be useful for random
registers that
> > > > > logically belong to one device but are grouped with other unrelated
> > > > > registers in a syscon, and that are in a different register
offset for
> > > > > each chip that has them. Using named properties instead of a list
> > > > > of phandle/arg tuples with names is clearly a simpler alternative
> > > > > and more like we do it today, but I can also see some API
simplification
> > > > > with Orson's patch without the binding getting out of hand.
> > > >
> > > > I understand when a phandle to a syscon is used. That's nothing new.
> > > > What's special about Unisoc SoC that needs something new/different?
> > > > I saw there's a large number of syscons, but I don't understand what's
> > > > in them.
> > > >
> > > > If the API is this:
> > > >
> > > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,
> > > > const char *name,
> > > > int arg_count, __u32 *out_args)
> > > >
> > > > How is 'name' being an entry in syscon-names simpler than
just being the
> > > > property name? The implementation for the latter would certainly be
> > > > simpler.
> > > >
> > > > It also makes the property unparseable without knowledge outside of the
> > > > DT (i.e. in the driver). I suppose if the number of cells for
each entry
> > > > is fixed, we could count the number of syscon-names entries to figure
> > > > out the stride. But then if one entry needs a lot of cells, then they
> > > > all have to have padding cells.
> > >
> > > Good point. The syscon_regmap_lookup_by_name() interface would
> > > work just as well when passing a property name compared to
> > > a name listed in another property, and this would still be more in
> > > line with what we do on other SoCs.
> > >
> >
> > udx710-modem.dtsi:69: syscons = <&pmu_apb_regs 0x18 0x2000000>,
> > udx710-modem.dtsi-70- <&pmu_apb_regs 0x544 0x1>,
> > udx710-modem.dtsi-71- <&aon_apb_regs 0x218 0x7e00>,
> > udx710-modem.dtsi-72- <&pmu_apb_regs 0xb0 0x20000>,
> > udx710-modem.dtsi-73- <&pmu_apb_regs 0xff 0x100>;
> > udx710-modem.dtsi:74: syscon-names = "shutdown", "deepsleep",
"corereset",
> > udx710-modem.dtsi-75- "sysreset", "getstatus";
>
> Reset at least has a standard binding.
You mean the reset-controller binding?
I have noticed that before.
Okay, it's time for us to consider to setup it.
>
>
> > ud710.dtsi:1268: syscons = <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
> > ud710.dtsi-1269- <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
> > ud710.dtsi-1270- <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG
MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
> > ud710.dtsi-1271- <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG
MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> > ud710.dtsi:1272: syscon-names = "sd_detect_pol",
> > ud710.dtsi-1273- "sd_hotplug_protect_en",
> > ud710.dtsi-1274- "sd_hotplug_debounce_en",
> > ud710.dtsi-1275- "sd_hotplug_debounce_cn";
>
> This looks to me like it should be a single phandle. How many different
> register layouts across how many SoCs do you need to support?
Not sure. It's up to the IC designer how to layout their next chip registers.
For a simple example:
Chip A: offset Chip B: offset
-------------------------+----------------------------
glb-reg0 0x0 | glb-reg0 0x0
glb-reg1 0x4 | a-new-reg 0x4 (newly insert in the
middle of list)
glb-reg2 0x8 | glb-reg1 0x8 (same function with A's reg1)
.... | glb-reg2 0xc (same function with A's reg2)
So for chip B's driver which is same to chip A, but the offsets are
partly different.
Earlier we use chip macro to separate them.
#ifdef CHIP_A
#define REG1 0x4
#endif
#ifdef CHIP_B
#define REG1 0x8
#endif
......
That time we also used single phandle like sprd,xxxx-syscon = <&yyy>;
But these macro will prevent us from building all-in-one kernel image
for different SoCs
and this also make dtb less powerful -- we can't use multiple dtbs
for different SoCs with a single image --
although these SoCs are compatible in many drivers.
>
> > > The only advantage I can see in having a list of phandle/arg tuples
> > > rather than a set of properties is that it is a slightly more compact
> > > representation in source form, but otherwise they should be equivalent
> >
> > Yes, I agree.
> > They are equivalent.
> >
> > But sprd SoCs have too many registers and the representation might matter.
> > Here's some real code from local,
> >
> > orca.dtsi:1276: syscons = <&pmu_apb_regs
REG_PMU_APB_RF_PD_AUDCP_SYS_CFG
MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >,
> > orca.dtsi-1277- <&pmu_apb_regs
REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG
MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>,
> > orca.dtsi-1278- <&pmu_apb_regs
REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>,
> > orca.dtsi-1279- <&pmu_apb_regs
REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>,
> > orca.dtsi-1280- <&pmu_apb_regs
REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>,
> > orca.dtsi-1281- <&pmu_apb_regs
REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>,
> > orca.dtsi-1282- <&pmu_apb_regs
REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>,
> > orca.dtsi-1283- <&pmu_apb_regs
REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>,
> > orca.dtsi-1284- <&pmu_apb_regs
REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>,
> > --
> > orca.dtsi:1288: syscon-names = "sysshutdown",
"coreshutdown", "deepsleep", "corereset",
> > orca.dtsi-1289- "sysreset", "reset_sel",
"sysstatus", "corestatus", "sleepstatus",
> > orca.dtsi-1290- "bootprotect", "bootvector",
"bootaddress_sel";
>
> Again, reset has standard binding.
>
> Also consider if you really need to access all of these vs. assuming a
> fixed mode that the firmware/bootloader sets up.
I believe most of them are used in kernel suspend/resume process.
>
> > ud710.dtsi:1268: syscons = <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>,
> > ud710.dtsi-1269- <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>,
> > ud710.dtsi-1270- <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG
MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>,
> > ud710.dtsi-1271- <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG
MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> > ud710.dtsi:1272: syscon-names = "sd_detect_pol",
> > ud710.dtsi-1273- "sd_hotplug_protect_en",
> > ud710.dtsi-1274- "sd_hotplug_debounce_en",
> > ud710.dtsi-1275- "sd_hotplug_debounce_cn";
> >
> > Compare to following,
> >
> > sd_hotplug_protect_en-syscon = <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>;
> > sd_hotplug_debounce_en-syscon = <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>;
> > sd_hotplug_debounce_cn-syscon = <&aon_apb_regs
REG_AON_APB_SDIO0_CTRL_REG
MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>;
> > .....
> >
> > For me, my choice would be the former.
> > It looks more clear.
> >
> > > and agree about this being harder to parse in an automated way.
> > >
> > > Orson, do you see any other reason for the combined property?
> > No other reason.
> >
> > > If not, could you respin the series once more with
> > > syscon_regmap_lookup_by_name() replaced by something like:?
> > >
> > > struct regmap *
> > > syscon_regmap_lookup_args_by_phandle(struct device_node *np,
> > > const char *property,
> > > int arg_count, __u32 *out_args)
> >
> > I like this idea. syscon_regmap_lookup_by_phandle_args() would be better?
> >
> > May I impelement them both?
>
> No.
Okay. I will send patch V3.
Thanks for your comments!
BR,
Orson
>
> Rob