[PATCH] mfd: syscon: fix syscon probing from dt

[PATCH] mfd: syscon: fix syscon probing from dt

[Philipp Zabel]

Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
breaks probing pure syscon devices from device tree, such as anatop and
iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
"syscon" compatible device tree nodes.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/mfd/syscon.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 176bf0f..70b8654 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -174,6 +174,11 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
 
+static const struct of_device_id of_syscon_match[] = {
+	{ .compatible = "syscon", },
+	{ },
+};
+
 static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -219,6 +224,7 @@ static const struct platform_device_id syscon_ids[] = {
 static struct platform_driver syscon_driver = {
 	.driver = {
 		.name = "syscon",
+		.of_match_table = of_syscon_match,
 	},
 	.probe		= syscon_probe,
 	.id_table	= syscon_ids,
-- 
2.1.4

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Heiko Stübner]

Hi Philipp,

Am Dienstag, 6. Januar 2015, 16:30:36 schrieb Philipp Zabel:
> Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform
> devices") breaks probing pure syscon devices from device tree, such as
> anatop and iomuxc-gpr on i.MX. This patch adds back the dt id table to
> match against "syscon" compatible device tree nodes.

could you elaborate a bit on the problem you're seeing without your patch?

With bdb0066df96e the syscon should be registered by the first call to one of 
the syscon_regmap_lookup_by_* functions. On my rockchip boards this works 
without any hickups:

       .bss : 0xc07f88a8 - 0xc0847264   ( 315 kB)
Hierarchical RCU implementation.
NR_IRQS:16 nr_irqs:16 16
L2C: failed to init: -19
	syscon_regmap_lookup_by_phandle of /syscon@ff770000
	of_syscon_register of /syscon@ff770000
Architected cp15 timer(s) running at 24.00MHz (phys).
sched_clock: 56 bits at 24MHz, resolution 41ns, wraps every 2863311519744ns

[... a lot later ...]

stmmaceth ff290000.ethernet: rk_gmac_setup: Can not read property: rx_delay.
stmmaceth ff290000.ethernet: rk_gmac_setup: set rx_delay to 0x10
	syscon_regmap_lookup_by_phandle of /syscon@ff770000
stmmaceth ff290000.ethernet: rk_gmac_setup: NO interface defined!
stmmaceth ff290000.ethernet: gmac_clk_init: clock input from PHY


The syscon@ff770000 from above also is a pure syscon device, so I'm wondering 
why it wouldn't work on imx boards.


Heiko

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Arnd Bergmann]

On Tuesday 06 January 2015 16:30:36 Philipp Zabel wrote:
> Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
> breaks probing pure syscon devices from device tree, such as anatop and
> iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
> "syscon" compatible device tree nodes.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 

I don't understand it. Why is this required?

	Arnd

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Philipp Zabel]

Hi Heiko,

Am Dienstag, den 06.01.2015, 20:05 +0100 schrieb Heiko Stübner:
> Hi Philipp,
> 
> Am Dienstag, 6. Januar 2015, 16:30:36 schrieb Philipp Zabel:
> > Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform
> > devices") breaks probing pure syscon devices from device tree, such as
> > anatop and iomuxc-gpr on i.MX. This patch adds back the dt id table to
> > match against "syscon" compatible device tree nodes.
> 
> could you elaborate a bit on the problem you're seeing without your patch?
>
> With bdb0066df96e the syscon should be registered by the first call to one of 
> the syscon_regmap_lookup_by_* functions. On my rockchip boards this works 
> without any hickups:

Oh, I didn't understand that is the way it's supposed to work now.

I noticed that after booting v3.19-rc on a nitrogen6x, the
/sys/kernel/debug/regmap/{20c8000.anatop,20e0000.iomuxc-gpr}
directories were missing. The syscon regmap is indeed registered,
but since regmap_init_mmio is called with dev = NULL, regmap_attach_dev,
and thus regmap_debugfs_init, is never called from regmap_init.

Also I want to add child devices to the syscon device since a while ago:
https://lkml.org/lkml/2014/12/15/133
This doesn't work anymore if there is no syscon device.

regards
Philipp

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Philipp Zabel]

Am Dienstag, den 06.01.2015, 20:36 +0100 schrieb Arnd Bergmann:
> On Tuesday 06 January 2015 16:30:36 Philipp Zabel wrote:
> > Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
> > breaks probing pure syscon devices from device tree, such as anatop and
> > iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
> > "syscon" compatible device tree nodes.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> 
> I don't understand it. Why is this required?

The debugfs entries vanished and I'd like to have a device to register
platform device children to. I guess for i.MX iomuxc it could be argued
that the iomuxc-gpr syscon should be merged into the iomuxc pinctrl
device instead of probing iomuxc-gpr as a platform device by itself.
How about allowing to register a syscon for a given device:

----8<----
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index d2280d6..2633b27 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -42,13 +42,14 @@ static struct regmap_config syscon_regmap_config = {
 	.reg_stride = 4,
 };
 
-static struct syscon *of_syscon_register(struct device_node *np)
+struct syscon *syscon_register(struct device *dev, struct device_node *np)
 {
 	struct syscon *syscon;
 	struct regmap *regmap;
 	void __iomem *base;
 	int ret;
 	struct regmap_config syscon_config = syscon_regmap_config;
+	struct resource res;
 
 	if (!of_device_is_compatible(np, "syscon"))
 		return ERR_PTR(-EINVAL);
@@ -57,7 +58,12 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	if (!syscon)
 		return ERR_PTR(-ENOMEM);
 
-	base = of_iomap(np, 0);
+	if (of_address_to_resource(np, 0, &res)) {
+		ret = -ENOMEM;
+		goto err_map;
+	}
+
+	base = ioremap(res.start, resource_size(&res));
 	if (!base) {
 		ret = -ENOMEM;
 		goto err_map;
@@ -69,7 +75,8 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	 else if (of_property_read_bool(np, "little-endian"))
 		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
 
-	regmap = regmap_init_mmio(NULL, base, &syscon_config);
+	syscon_regmap_config.max_register = res.end - res.start - 3;
+	regmap = regmap_init_mmio(dev, base, &syscon_config);
 	if (IS_ERR(regmap)) {
 		pr_err("regmap init failed\n");
 		ret = PTR_ERR(regmap);
@@ -91,6 +98,12 @@ err_map:
 	kfree(syscon);
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_GPL(syscon_register);
+
+static struct syscon *of_syscon_register(struct device_node *np)
+{
+	return syscon_register(NULL, np);
+}
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 75e543b..e0c4a86 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -17,10 +17,14 @@
 
 #include <linux/err.h>
 
+struct device;
 struct device_node;
+struct syscon;
 
 #ifdef CONFIG_MFD_SYSCON
 extern struct regmap *syscon_node_to_regmap(struct device_node *np);
+extern struct syscon *syscon_register(struct device *dev,
+				      struct device_node *np);
 extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
 extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
 extern struct regmap *syscon_regmap_lookup_by_phandle(
@@ -32,6 +36,12 @@ static inline struct regmap *syscon_node_to_regmap(struct device_node *np)
 	return ERR_PTR(-ENOSYS);
 }
 
+static struct syscon *syscon_register(struct device *dev,
+				      struct device_node *np)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 {
 	return ERR_PTR(-ENOSYS);
--
---->8----

That way the syscon could be registered from iomuxc (pinctrl-imx6q):

----8<----
diff --git a/drivers/pinctrl/freescale/pinctrl-imx6q.c b/drivers/pinctrl/freescale/pinctrl-imx6q.c
index 4d1fcb8..74a68ec 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx6q.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx6q.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -473,6 +474,12 @@ static const struct of_device_id imx6q_pinctrl_of_match[] = {
 
 static int imx6q_pinctrl_probe(struct platform_device *pdev)
 {
+	struct device_node *syscon_np;
+
+	syscon_np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-iomuxc-gpr");
+	if (syscon_np)
+		syscon_register(&pdev->dev, syscon_np);
+
 	return imx_pinctrl_probe(pdev, &imx6q_pinctrl_info);
 }
 
---->8----

which makes the regmap debugfs entry return
as /sys/kernel/debug/regmap/20e0000.iomuxc
and allows to register children to the iomuxc-gpr node.

regards
Philipp

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Pankaj Dubey]

Hi Philipp,

On Wednesday 07 January 2015 04:27 PM, Philipp Zabel wrote:
> Am Dienstag, den 06.01.2015, 20:36 +0100 schrieb Arnd Bergmann:
>> On Tuesday 06 January 2015 16:30:36 Philipp Zabel wrote:
>>> Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
>>> breaks probing pure syscon devices from device tree, such as anatop and
>>> iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
>>> "syscon" compatible device tree nodes.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>
>>
>> I don't understand it. Why is this required?
>
> The debugfs entries vanished and I'd like to have a device to register
> platform device children to.

Yes debugfs entries will not be present for syscon, this is as per 
discussion happened here [1]. At that time one suggestion came from Arnd 
that we can have a different representation for syscon regmaps, instead 
of based on devices just based on of_node pointer.

1: https://lkml.org/lkml/2014/9/26/73

I was just thinking, missing debugfs entry is your concern, or you want 
to make child devices of syscon devices? Basically I am still trying to 
understand your requirement.

I guess for i.MX iomuxc it could be argued
> that the iomuxc-gpr syscon should be merged into the iomuxc pinctrl
> device instead of probing iomuxc-gpr as a platform device by itself.
> How about allowing to register a syscon for a given device:
>
> ----8<----
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index d2280d6..2633b27 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -42,13 +42,14 @@ static struct regmap_config syscon_regmap_config = {
>   	.reg_stride = 4,
>   };
>
> -static struct syscon *of_syscon_register(struct device_node *np)
> +struct syscon *syscon_register(struct device *dev, struct device_node *np)
>   {
>   	struct syscon *syscon;
>   	struct regmap *regmap;
>   	void __iomem *base;
>   	int ret;
>   	struct regmap_config syscon_config = syscon_regmap_config;
> +	struct resource res;
>
>   	if (!of_device_is_compatible(np, "syscon"))
>   		return ERR_PTR(-EINVAL);
> @@ -57,7 +58,12 @@ static struct syscon *of_syscon_register(struct device_node *np)
>   	if (!syscon)
>   		return ERR_PTR(-ENOMEM);
>
> -	base = of_iomap(np, 0);
> +	if (of_address_to_resource(np, 0, &res)) {
> +		ret = -ENOMEM;
> +		goto err_map;
> +	}
> +
> +	base = ioremap(res.start, resource_size(&res));
>   	if (!base) {
>   		ret = -ENOMEM;
>   		goto err_map;
> @@ -69,7 +75,8 @@ static struct syscon *of_syscon_register(struct device_node *np)
>   	 else if (of_property_read_bool(np, "little-endian"))
>   		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
>
> -	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> +	syscon_regmap_config.max_register = res.end - res.start - 3;
> +	regmap = regmap_init_mmio(dev, base, &syscon_config);
>   	if (IS_ERR(regmap)) {
>   		pr_err("regmap init failed\n");
>   		ret = PTR_ERR(regmap);
> @@ -91,6 +98,12 @@ err_map:
>   	kfree(syscon);
>   	return ERR_PTR(ret);
>   }
> +EXPORT_SYMBOL_GPL(syscon_register);
> +

In past a similar approach [2] for letting device driver to register 
themselves as syscon provided, has been rejected giving reason that 
syscon should not need it's own platform device.

2: 
https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg35744.html


Thanks,
Pankaj Dubey
> +static struct syscon *of_syscon_register(struct device_node *np)
> +{
> +	return syscon_register(NULL, np);
> +}
>
>   struct regmap *syscon_node_to_regmap(struct device_node *np)
>   {
> diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
> index 75e543b..e0c4a86 100644
> --- a/include/linux/mfd/syscon.h
> +++ b/include/linux/mfd/syscon.h
> @@ -17,10 +17,14 @@
>
>   #include <linux/err.h>
>
> +struct device;
>   struct device_node;
> +struct syscon;
>
>   #ifdef CONFIG_MFD_SYSCON
>   extern struct regmap *syscon_node_to_regmap(struct device_node *np);
> +extern struct syscon *syscon_register(struct device *dev,
> +				      struct device_node *np);
>   extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
>   extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
>   extern struct regmap *syscon_regmap_lookup_by_phandle(
> @@ -32,6 +36,12 @@ static inline struct regmap *syscon_node_to_regmap(struct device_node *np)
>   	return ERR_PTR(-ENOSYS);
>   }
>
> +static struct syscon *syscon_register(struct device *dev,
> +				      struct device_node *np)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
>   static inline struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
>   {
>   	return ERR_PTR(-ENOSYS);
> --
> ---->8----
>
> That way the syscon could be registered from iomuxc (pinctrl-imx6q):
>
> ----8<----
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx6q.c b/drivers/pinctrl/freescale/pinctrl-imx6q.c
> index 4d1fcb8..74a68ec 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx6q.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx6q.c
> @@ -15,6 +15,7 @@
>   #include <linux/err.h>
>   #include <linux/init.h>
>   #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_device.h>
> @@ -473,6 +474,12 @@ static const struct of_device_id imx6q_pinctrl_of_match[] = {
>
>   static int imx6q_pinctrl_probe(struct platform_device *pdev)
>   {
> +	struct device_node *syscon_np;
> +
> +	syscon_np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-iomuxc-gpr");
> +	if (syscon_np)
> +		syscon_register(&pdev->dev, syscon_np);
> +
>   	return imx_pinctrl_probe(pdev, &imx6q_pinctrl_info);
>   }
>
> ---->8----
>
> which makes the regmap debugfs entry return
> as /sys/kernel/debug/regmap/20e0000.iomuxc
> and allows to register children to the iomuxc-gpr node.
>
> regards
> Philipp
>
>

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Philipp Zabel]

Hi Pankaj,

Am Mittwoch, den 07.01.2015, 16:47 +0530 schrieb Pankaj Dubey:
> Hi Philipp,
> 
> On Wednesday 07 January 2015 04:27 PM, Philipp Zabel wrote:
> > Am Dienstag, den 06.01.2015, 20:36 +0100 schrieb Arnd Bergmann:
> >> On Tuesday 06 January 2015 16:30:36 Philipp Zabel wrote:
> >>> Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
> >>> breaks probing pure syscon devices from device tree, such as anatop and
> >>> iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
> >>> "syscon" compatible device tree nodes.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>>
> >>
> >> I don't understand it. Why is this required?
> >
> > The debugfs entries vanished and I'd like to have a device to register
> > platform device children to.
> 
> Yes debugfs entries will not be present for syscon, this is as per 
> discussion happened here [1]. At that time one suggestion came from Arnd 
> that we can have a different representation for syscon regmaps, instead 
> of based on devices just based on of_node pointer.
> 
> 1: https://lkml.org/lkml/2014/9/26/73

Thank you for the pointer. That would work for me just as well.

> I was just thinking, missing debugfs entry is your concern, or you want 
> to make child devices of syscon devices? Basically I am still trying to 
> understand your requirement.

Both. I'd like to somehow get the debug entry back, whether by
optionally associating syscon with a device or by adding special syscon
support to regmap.
And I'd like to eventually register platform devices under the syscon
node in the device tree. Currently in the i.MX device tree we have three
overlapping nodes next to each other:

	gpr: iomuxc-gpr@20e00000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;
	};

	iomuxc: iomuxc@020e0000 {
		compatible = "fsl,imx6q-iomuxc";
		reg = <0x020e0000 0x4000>;
	};

	ldb: ldb@020e0008 {
		compatible = "fsl,imx6q-ldb";
		gpr = <&gpr>;
	};

The ldb device is controlled purely through the gpr regmap, so in my
opinion it should be a child to the iomuxc-gpr node:

	gpr: iomuxc-gpr@20e00000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;

		ldb: ldb@020e0008 {
			compatible = "fsl,imx6q-ldb";
			gpr = <&gpr>;
		};
	};

	iomuxc: iomuxc@020e0000 {
		compatible = "fsl,imx6q-iomuxc";
		reg = <0x020e0000 0x4000>;
	};

In that case, if there is no device associated with iomuxc-gpr, which
device should be the parent to ldb? If the syscon is created from the
iomuxc device, the ldb device could be made a child to iomuxc.
In fact, since the gpr registers are part of the iomuxc register space,
one could even think about merging the iomuxg-gpr syscon into the iomuxc
node:

	gpr: iomuxc: iomuxc@020e0000 {
		compatible = "fsl,imx6q-iomuxc", "fsl,imx6q-iomuxc-gpr";
		reg = <0x020e0000 0x4000>;

		ldb: ldb@020e0008 {
			compatible = "fsl,imx6q-ldb";
		};
	};

> I guess for i.MX iomuxc it could be argued
> > that the iomuxc-gpr syscon should be merged into the iomuxc pinctrl
> > device instead of probing iomuxc-gpr as a platform device by itself.
> > How about allowing to register a syscon for a given device:
> >
> > ----8<----
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index d2280d6..2633b27 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -42,13 +42,14 @@ static struct regmap_config syscon_regmap_config = {
> >   	.reg_stride = 4,
> >   };
> >
> > -static struct syscon *of_syscon_register(struct device_node *np)
> > +struct syscon *syscon_register(struct device *dev, struct device_node *np)
> >   {
> >   	struct syscon *syscon;
> >   	struct regmap *regmap;
> >   	void __iomem *base;
> >   	int ret;
> >   	struct regmap_config syscon_config = syscon_regmap_config;
> > +	struct resource res;
> >
> >   	if (!of_device_is_compatible(np, "syscon"))
> >   		return ERR_PTR(-EINVAL);
> > @@ -57,7 +58,12 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >   	if (!syscon)
> >   		return ERR_PTR(-ENOMEM);
> >
> > -	base = of_iomap(np, 0);
> > +	if (of_address_to_resource(np, 0, &res)) {
> > +		ret = -ENOMEM;
> > +		goto err_map;
> > +	}
> > +
> > +	base = ioremap(res.start, resource_size(&res));
> >   	if (!base) {
> >   		ret = -ENOMEM;
> >   		goto err_map;
> > @@ -69,7 +75,8 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >   	 else if (of_property_read_bool(np, "little-endian"))
> >   		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> >
> > -	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> > +	syscon_regmap_config.max_register = res.end - res.start - 3;
> > +	regmap = regmap_init_mmio(dev, base, &syscon_config);
> >   	if (IS_ERR(regmap)) {
> >   		pr_err("regmap init failed\n");
> >   		ret = PTR_ERR(regmap);
> > @@ -91,6 +98,12 @@ err_map:
> >   	kfree(syscon);
> >   	return ERR_PTR(ret);
> >   }
> > +EXPORT_SYMBOL_GPL(syscon_register);
> > +
> 
> In past a similar approach [2] for letting device driver to register 
> themselves as syscon provided, has been rejected giving reason that 
> syscon should not need it's own platform device.

In that mail Arnd mentions registering a syscon with both an optional
device and an explicit device_node pointer, which is what this patch
would do. There is no need for a platform device.

regards
Philipp

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Pankaj Dubey]

Hi Philipp,

On Wednesday 07 January 2015 05:25 PM, Philipp Zabel wrote:
> Hi Pankaj,
>
> Am Mittwoch, den 07.01.2015, 16:47 +0530 schrieb Pankaj Dubey:
>> Hi Philipp,
>>
>> On Wednesday 07 January 2015 04:27 PM, Philipp Zabel wrote:
>>> Am Dienstag, den 06.01.2015, 20:36 +0100 schrieb Arnd Bergmann:
>>>> On Tuesday 06 January 2015 16:30:36 Philipp Zabel wrote:
>>>>> Patch bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
>>>>> breaks probing pure syscon devices from device tree, such as anatop and
>>>>> iomuxc-gpr on i.MX. This patch adds back the dt id table to match against
>>>>> "syscon" compatible device tree nodes.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>>>
>>>>
>>>> I don't understand it. Why is this required?
>>>
>>> The debugfs entries vanished and I'd like to have a device to register
>>> platform device children to.
>>
>> Yes debugfs entries will not be present for syscon, this is as per
>> discussion happened here [1]. At that time one suggestion came from Arnd
>> that we can have a different representation for syscon regmaps, instead
>> of based on devices just based on of_node pointer.
>>
>> 1: https://lkml.org/lkml/2014/9/26/73
>
> Thank you for the pointer. That would work for me just as well.
>

Well, But so far I do not see any attempt has been done for making this 
kind of debugfs entry using of_node pointer.

>> I was just thinking, missing debugfs entry is your concern, or you want
>> to make child devices of syscon devices? Basically I am still trying to
>> understand your requirement.
>
> Both. I'd like to somehow get the debug entry back, whether by
> optionally associating syscon with a device or by adding special syscon
> support to regmap.
> And I'd like to eventually register platform devices under the syscon
> node in the device tree. Currently in the i.MX device tree we have three
> overlapping nodes next to each other:
>
> 	gpr: iomuxc-gpr@20e00000 {
> 		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> 		reg = <0x020e0000 0x38>;
> 	};
>
> 	iomuxc: iomuxc@020e0000 {
> 		compatible = "fsl,imx6q-iomuxc";
> 		reg = <0x020e0000 0x4000>;
> 	};
>
> 	ldb: ldb@020e0008 {
> 		compatible = "fsl,imx6q-ldb";
> 		gpr = <&gpr>;
> 	};
>
> The ldb device is controlled purely through the gpr regmap, so in my
> opinion it should be a child to the iomuxc-gpr node:
>
> 	gpr: iomuxc-gpr@20e00000 {
> 		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> 		reg = <0x020e0000 0x38>;
>
> 		ldb: ldb@020e0008 {
> 			compatible = "fsl,imx6q-ldb";
> 			gpr = <&gpr>;
> 		};
> 	};
>
> 	iomuxc: iomuxc@020e0000 {
> 		compatible = "fsl,imx6q-iomuxc";
> 		reg = <0x020e0000 0x4000>;
> 	};
>
> In that case, if there is no device associated with iomuxc-gpr, which
> device should be the parent to ldb?If the syscon is created from the
> iomuxc device, the ldb device could be made a child to iomuxc.

If you need ldb as child device of gpr, then there is no need of syscon 
or SYSCON APIs here in ldb. In my opinion this is an extra feature which 
are trying to feed into syscon. As in case of sibling devices it makes 
sense to access registers across them using syscon APIs, but in case of 
parent-child devices we already have MFD drivers, and MFD client devices 
which can serve this purpose.

> In fact, since the gpr registers are part of the iomuxc register space,
> one could even think about merging the iomuxg-gpr syscon into the iomuxc
> node:
>
> 	gpr: iomuxc: iomuxc@020e0000 {
> 		compatible = "fsl,imx6q-iomuxc", "fsl,imx6q-iomuxc-gpr";
> 		reg = <0x020e0000 0x4000>;
>
> 		ldb: ldb@020e0008 {
> 			compatible = "fsl,imx6q-ldb";
> 		};
> 	};
>
>> I guess for i.MX iomuxc it could be argued
>>> that the iomuxc-gpr syscon should be merged into the iomuxc pinctrl
>>> device instead of probing iomuxc-gpr as a platform device by itself.

Yes, also in the probe of this driver you can register itself as MFD 
driver and all the child devices and MFD client devices so that child 
devices can be probed. For example a similar approach has been attempted 
for exynos-pmu here [1].

1: http://www.spinics.net/lists/linux-samsung-soc/msg38446.html

While doing this you can still keep "syscon" compatible to the gpr node 
if it's some of it's register will be accessed from some other devices.
But definitely ldb does not need to have a phandle to this syscon 
device, as after being a MFD client device it can access MFD parent 
device address space.

>>> How about allowing to register a syscon for a given device:
>>>
>>> ----8<----
>>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>>> index d2280d6..2633b27 100644
>>> --- a/drivers/mfd/syscon.c
>>> +++ b/drivers/mfd/syscon.c
>>> @@ -42,13 +42,14 @@ static struct regmap_config syscon_regmap_config = {
>>>    	.reg_stride = 4,
>>>    };
>>>
>>> -static struct syscon *of_syscon_register(struct device_node *np)
>>> +struct syscon *syscon_register(struct device *dev, struct device_node *np)
>>>    {
>>>    	struct syscon *syscon;
>>>    	struct regmap *regmap;
>>>    	void __iomem *base;
>>>    	int ret;
>>>    	struct regmap_config syscon_config = syscon_regmap_config;
>>> +	struct resource res;
>>>
>>>    	if (!of_device_is_compatible(np, "syscon"))
>>>    		return ERR_PTR(-EINVAL);
>>> @@ -57,7 +58,12 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>>    	if (!syscon)
>>>    		return ERR_PTR(-ENOMEM);
>>>
>>> -	base = of_iomap(np, 0);
>>> +	if (of_address_to_resource(np, 0, &res)) {
>>> +		ret = -ENOMEM;
>>> +		goto err_map;
>>> +	}
>>> +
>>> +	base = ioremap(res.start, resource_size(&res));
>>>    	if (!base) {
>>>    		ret = -ENOMEM;
>>>    		goto err_map;
>>> @@ -69,7 +75,8 @@ static struct syscon *of_syscon_register(struct device_node *np)
>>>    	 else if (of_property_read_bool(np, "little-endian"))
>>>    		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
>>>
>>> -	regmap = regmap_init_mmio(NULL, base, &syscon_config);
>>> +	syscon_regmap_config.max_register = res.end - res.start - 3;
>>> +	regmap = regmap_init_mmio(dev, base, &syscon_config);
>>>    	if (IS_ERR(regmap)) {
>>>    		pr_err("regmap init failed\n");
>>>    		ret = PTR_ERR(regmap);
>>> @@ -91,6 +98,12 @@ err_map:
>>>    	kfree(syscon);
>>>    	return ERR_PTR(ret);
>>>    }
>>> +EXPORT_SYMBOL_GPL(syscon_register);
>>> +
>>
>> In past a similar approach [2] for letting device driver to register
>> themselves as syscon provided, has been rejected giving reason that
>> syscon should not need it's own platform device.
>
> In that mail Arnd mentions registering a syscon with both an optional
> device and an explicit device_node pointer, which is what this patch
> would do. There is no need for a platform device.
>

I don't think so, as per I recall he was against of any such extra API 
to register syscon [2].

2: https://lkml.org/lkml/2014/9/1/227

> regards
> Philipp
>
>

Re: [PATCH] mfd: syscon: fix syscon probing from dt

[Philipp Zabel]

Hi Pankaj,

Am Donnerstag, den 08.01.2015, 16:37 +0530 schrieb Pankaj Dubey:
[...]
> > The ldb device is controlled purely through the gpr regmap, so in my
> > opinion it should be a child to the iomuxc-gpr node:
> >
> > 	gpr: iomuxc-gpr@20e00000 {
> > 		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > 		reg = <0x020e0000 0x38>;
> >
> > 		ldb: ldb@020e0008 {
> > 			compatible = "fsl,imx6q-ldb";
> > 			gpr = <&gpr>;
> > 		};
> > 	};
> >
> > 	iomuxc: iomuxc@020e0000 {
> > 		compatible = "fsl,imx6q-iomuxc";
> > 		reg = <0x020e0000 0x4000>;
> > 	};
> >
> > In that case, if there is no device associated with iomuxc-gpr, which
> > device should be the parent to ldb?If the syscon is created from the
> > iomuxc device, the ldb device could be made a child to iomuxc.
> 
> If you need ldb as child device of gpr, then there is no need of syscon 
> or SYSCON APIs here in ldb. In my opinion this is an extra feature which 
> are trying to feed into syscon. As in case of sibling devices it makes 
> sense to access registers across them using syscon APIs, but in case of 
> parent-child devices we already have MFD drivers, and MFD client devices 
> which can serve this purpose.

ldb is not the only device controlled purely through the GPR region, and
the multiplexers are in a shared register, for example, so these
registers should be accessed through a shared regmap.

syscon is a mechanism to share a regmap between multiple devices.
Whether they are siblings or parent/child or in completely different
places in the device tree is somewhat besides the point.

MFD is for devices with multiple functional blocks, but this is not what
the GPR region is. This register space just bundles various signals from
different parts of the SoC. A few of these signals happen to be the only
way to talk to the ldb.

> > In fact, since the gpr registers are part of the iomuxc register space,
> > one could even think about merging the iomuxg-gpr syscon into the iomuxc
> > node:
> >
> > 	gpr: iomuxc: iomuxc@020e0000 {
> > 		compatible = "fsl,imx6q-iomuxc", "fsl,imx6q-iomuxc-gpr";
> > 		reg = <0x020e0000 0x4000>;
> >
> > 		ldb: ldb@020e0008 {
> > 			compatible = "fsl,imx6q-ldb";
> > 		};
> > 	};
> >
> >> I guess for i.MX iomuxc it could be argued
> >>> that the iomuxc-gpr syscon should be merged into the iomuxc pinctrl
> >>> device instead of probing iomuxc-gpr as a platform device by itself.
> 
> Yes, also in the probe of this driver you can register itself as MFD 
> driver and all the child devices and MFD client devices so that child 
> devices can be probed. For example a similar approach has been attempted 
> for exynos-pmu here [1].

I disagree. MFD is the wrong abstraction as there are no cells in which
to partition the GPR, and the children are not allowed to do MMIO access
themselves. All the useful parts of MFD would serve no purpose here.

> 1: http://www.spinics.net/lists/linux-samsung-soc/msg38446.html
> 
> While doing this you can still keep "syscon" compatible to the gpr node 
> if it's some of it's register will be accessed from some other devices.
> But definitely ldb does not need to have a phandle to this syscon 
> device, as after being a MFD client device it can access MFD parent 
> device address space.

[...]
> >> In past a similar approach [2] for letting device driver to register
> >> themselves as syscon provided, has been rejected giving reason that
> >> syscon should not need it's own platform device.
> >
> > In that mail Arnd mentions registering a syscon with both an optional
> > device and an explicit device_node pointer, which is what this patch
> > would do. There is no need for a platform device.
> >
> 
> I don't think so, as per I recall he was against of any such extra API 
> to register syscon [2].
> 
> 2: https://lkml.org/lkml/2014/9/1/227

This comment is about removing the dependency on a platform driver.
My patch doesn't touch that. It just adds an optional device pointer,
which is completely different.

regards
Philipp