of_syscon_register_regmap() was added for nodes which need a custom
regmap setup. It's not really correct for those nodes to claim they are
compatible with "syscon" as the default handling likely doesn't work in
those cases. If device_node_get_regmap() happens to be called first,
then of_syscon_register() will be called and an incorrect regmap will be
created (barring some other error). That may lead to unknown results in
the worst case. In the best case, of_syscon_register_regmap() will fail
with -EEXIST. This problem remains unless these cases drop "syscon" (an
ABI issue) or we exclude them using their specific compatible. ATM,
there is only one user: "google,gs101-pmu"

There are also cases of adding "syscon" compatible to existing nodes
after the fact in order to register the syscon. That presents a
potential DT ABI problem. Instead, if there's a kernel change needing a
syscon for a node, then it should be possible to allow the kernel to
register a syscon without a DT change. That's only possible by using
of_syscon_register_regmap() currently, but in the future we may want to
support a match list for cases which don't need a custom regmap.

With this change, the lookup functions will succeed for any node
registered by of_syscon_register_regmap() regardless of whether the node
compatible contains "syscon".

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Fix logic when a syscon is found in list to not return an error
---
 drivers/mfd/syscon.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 			break;
 		}
 
-	if (!syscon)
-		syscon = of_syscon_register(np, check_res);
-
+	if (!syscon) {
+		if (of_device_is_compatible(np, "syscon"))
+			syscon = of_syscon_register(np, check_res);
+		else
+			syscon = ERR_PTR(-EINVAL);
+	}
 	mutex_unlock(&syscon_list_lock);
 
 	if (IS_ERR(syscon))
@@ -238,9 +241,6 @@ EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
-	if (!of_device_is_compatible(np, "syscon"))
-		return ERR_PTR(-EINVAL);
-
 	return device_node_get_regmap(np, true);
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);

-- 
2.45.2

Hi Rob,

On 17/12/24 23:41, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes
> after the fact in order to register the syscon. That presents a
> potential DT ABI problem. Instead, if there's a kernel change needing a
> syscon for a node, then it should be possible to allow the kernel to
> register a syscon without a DT change. That's only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node
> registered by of_syscon_register_regmap() regardless of whether the node
> compatible contains "syscon".
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v2:
>   - Fix logic when a syscon is found in list to not return an error
> ---
>   drivers/mfd/syscon.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
>   			break;
>   		}
>   
> -	if (!syscon)
> -		syscon = of_syscon_register(np, check_res);
> -
> +	if (!syscon) {
> +		if (of_device_is_compatible(np, "syscon"))
> +			syscon = of_syscon_register(np, check_res);
> +		else
> +			syscon = ERR_PTR(-EINVAL);
> +	}

Earlier the above check was only in syscon_node_to_regmap() , but now 
the users of device_node_to_regmap() also undergoes this check and there 
are few drivers in tree which uses device_node_to_regmap() without 
having "syscon" in compatible, likely those drivers need to be fixed, 
but this patch breaks all those drivers now, atleast the below drivers 
are affected and all TI platforms fail to boot due to this:

drivers/soc/ti/k3-socinfo.c
drivers/mux/mmio.c ("reg-mux" users)
drivers/clk/keystone/syscon-clk.c

All the above drivers fail due to the above check for syscon compatible.

What is the recommended solution to fix this?

Thanks and Regards,
Vaishnav

>   	mutex_unlock(&syscon_list_lock);
>   
>   	if (IS_ERR(syscon))
> @@ -238,9 +241,6 @@ EXPORT_SYMBOL_GPL(device_node_to_regmap);
>   
>   struct regmap *syscon_node_to_regmap(struct device_node *np)
>   {
> -	if (!of_device_is_compatible(np, "syscon"))
> -		return ERR_PTR(-EINVAL);
> -
>   	return device_node_get_regmap(np, true);
>   }
>   EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
>

On Wed, Jan 22, 2025 at 3:44 AM Vaishnav Achath <vaishnav.a@ti.com> wrote:
>
> Hi Rob,
>
> On 17/12/24 23:41, Rob Herring (Arm) wrote:
> > of_syscon_register_regmap() was added for nodes which need a custom
> > regmap setup. It's not really correct for those nodes to claim they are
> > compatible with "syscon" as the default handling likely doesn't work in
> > those cases. If device_node_get_regmap() happens to be called first,
> > then of_syscon_register() will be called and an incorrect regmap will be
> > created (barring some other error). That may lead to unknown results in
> > the worst case. In the best case, of_syscon_register_regmap() will fail
> > with -EEXIST. This problem remains unless these cases drop "syscon" (an
> > ABI issue) or we exclude them using their specific compatible. ATM,
> > there is only one user: "google,gs101-pmu"
> >
> > There are also cases of adding "syscon" compatible to existing nodes
> > after the fact in order to register the syscon. That presents a
> > potential DT ABI problem. Instead, if there's a kernel change needing a
> > syscon for a node, then it should be possible to allow the kernel to
> > register a syscon without a DT change. That's only possible by using
> > of_syscon_register_regmap() currently, but in the future we may want to
> > support a match list for cases which don't need a custom regmap.
> >
> > With this change, the lookup functions will succeed for any node
> > registered by of_syscon_register_regmap() regardless of whether the node
> > compatible contains "syscon".
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > v2:
> >   - Fix logic when a syscon is found in list to not return an error
> > ---
> >   drivers/mfd/syscon.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
> >                       break;
> >               }
> >
> > -     if (!syscon)
> > -             syscon = of_syscon_register(np, check_res);
> > -
> > +     if (!syscon) {
> > +             if (of_device_is_compatible(np, "syscon"))
> > +                     syscon = of_syscon_register(np, check_res);
> > +             else
> > +                     syscon = ERR_PTR(-EINVAL);
> > +     }
>
> Earlier the above check was only in syscon_node_to_regmap() , but now
> the users of device_node_to_regmap() also undergoes this check and there
> are few drivers in tree which uses device_node_to_regmap() without
> having "syscon" in compatible, likely those drivers need to be fixed,
> but this patch breaks all those drivers now, atleast the below drivers
> are affected and all TI platforms fail to boot due to this:
>
> drivers/soc/ti/k3-socinfo.c
> drivers/mux/mmio.c ("reg-mux" users)
> drivers/clk/keystone/syscon-clk.c
>
> All the above drivers fail due to the above check for syscon compatible.
>
> What is the recommended solution to fix this?

I've sent a fix[1]. That's not to say I don't agree with fixes to
drivers removing device_node_to_regmap() calls. If you just need a
regmap for within a single driver, then just use regmap APIs directly.
What's in syscon.c is for (surprise!) syscon's, and those have the
characteristic of being shared.

Rob

[1] https://lore.kernel.org/all/20250124191644.2309790-1-robh@kernel.org/

On 17/12/2024 19:11, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

> -----Original Message-----
> From: Rob Herring (Arm) <robh@kernel.org>
> Sent: Tuesday, December 17, 2024 11:42 PM
> To: Lee Jones <lee@kernel.org>; Arnd Bergmann <arnd@arndb.de>; Pankaj
> Dubey <pankaj.dubey@samsung.com>; Heiko Stuebner <heiko@sntech.de>;
> Liviu Dudau <liviu.dudau@arm.com>; Sudeep Holla <sudeep.holla@arm.com>;
> Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Peter Griffin <peter.griffin@linaro.org>; Will McVicker
> <willmcvicker@google.com>; John Madieu <john.madieu.xa@bp.renesas.com>;
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon"
> compatible
> 
> of_syscon_register_regmap() was added for nodes which need a custom regmap
> setup. It's not really correct for those nodes to claim they are compatible with
> "syscon" as the default handling likely doesn't work in those cases. If
> device_node_get_regmap() happens to be called first, then of_syscon_register()
> will be called and an incorrect regmap will be created (barring some other error).
> That may lead to unknown results in the worst case. In the best case,
> of_syscon_register_regmap() will fail with -EEXIST. This problem remains unless
> these cases drop "syscon" (an ABI issue) or we exclude them using their specific
> compatible. ATM, there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes after the
> fact in order to register the syscon. That presents a potential DT ABI problem.
> Instead, if there's a kernel change needing a syscon for a node, then it should be
> possible to allow the kernel to register a syscon without a DT change. That's
> only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node registered by
> of_syscon_register_regmap() regardless of whether the node compatible
> contains "syscon".
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---

Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>

Thanks,
Pankaj Dubey

On 12/17/2024, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes
> after the fact in order to register the syscon. That presents a
> potential DT ABI problem. Instead, if there's a kernel change needing a
> syscon for a node, then it should be possible to allow the kernel to
> register a syscon without a DT change. That's only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node
> registered by of_syscon_register_regmap() regardless of whether the node
> compatible contains "syscon".
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

I verified this works on my Pixel 6. Thanks!

Tested-by: Will McVicker <willmcvicker@google.com>

Thanks,
Will