[PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Pankaj Dubey]

Currently a syscon entity can be only registered directly through a
platform device that binds to a dedicated syscon driver. However in
certain use cases it is desirable to make a device used with another
driver a syscon interface provider.

For example, certain SoCs (e.g. Exynos) contain system controller
blocks which perform various functions such as power domain control,
CPU power management, low power mode control, but in addition contain
certain IP integration glue, such as various signal masks,
coprocessor power control, etc. In such case, there is a need to have
a dedicated driver for such system controller but also share registers
with other drivers. The latter is where the syscon interface is helpful.

In case of DT based platforms, this patch decouples syscon object from
syscon platform driver, and allows to create syscon objects first time
when it is required by calling of syscon_regmap_lookup_by APIs and keep
a list of such syscon objects along with syscon provider device_nodes
and regmap handles.

For non-DT based platforms, this patch keeps syscon platform driver
structure where is can be probed and such non-DT based drivers can use
syscon_regmap_lookup_by_pdev API and get access to regmap handles.
Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
we can completly remove platform driver of syscon, and keep only helper
functions to get regmap handles.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
Patch v4 and related discussions can be found here [1].

Changes since v4:
 - Addressed Tomasz Figa's comments for v4.
 - Added error handing in of_syscon_register function.
 - Using devm_regmap_init_mmio instead of regmap_init_mmio.

Changes since v3:
 - Addressed Arnd's comment for v2.
 - Updated of_syscon_register for adding dev pointer in regmap_init_mmio.
 - For early users created dummy platform device.
   
Changes since v2:
 - Added back platform device support from syscon, with one change that
   syscon will not be probed for DT based platform.
 - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base
   users of syscon will not be broken.
 - Removed unwanted change in syscon.h.
 - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and
   Arnd Bergmann.
 - Added Tested-by of Vivek Gautam for testing on Exynos platform.

Changes since v1:
 - Removed of_syscon_unregister function.
 - Modified of_syscon_register function and it will be used by syscon.c
   to create syscon objects whenever required.
 - Removed platform device support from syscon.
 - Removed syscon_regmap_lookup_by_pdevname API support.
 - As there are significant changes w.r.t patchset v1, I am taking over
   author for this patchset from Tomasz Figa.

[1]: https://lkml.org/lkml/2014/9/19/267

 drivers/mfd/syscon.c |  107 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 85 insertions(+), 22 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index ca15878..0b17f50 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/list.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
@@ -22,31 +23,105 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
+#include <linux/slab.h>
 
 static struct platform_driver syscon_driver;
 
+static DEFINE_SPINLOCK(syscon_list_slock);
+static LIST_HEAD(syscon_list);
+
 struct syscon {
+	struct device_node *np;
 	struct regmap *regmap;
+	struct list_head list;
+};
+
+static struct regmap_config syscon_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
 };
 
-static int syscon_match_node(struct device *dev, void *data)
+static struct syscon *of_syscon_register(struct device_node *np)
 {
-	struct device_node *dn = data;
+	struct platform_device *pdev = NULL;
+	struct syscon *syscon;
+	struct regmap *regmap;
+	void __iomem *base;
+	int ret;
+
+	if (!of_device_is_compatible(np, "syscon"))
+		return ERR_PTR(-EINVAL);
+
+	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+	if (!syscon)
+		return ERR_PTR(-ENOMEM);
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		ret = -ENOMEM;
+		goto err_map;
+	}
+
+	/* if device is already populated and available then use it */
+	pdev = of_find_device_by_node(np);
+	if (!pdev) {
+		/* for early users create dummy syscon device and use it */
+		pdev = platform_device_alloc("dummy-syscon", -1);
+		if (!pdev) {
+			ret = -ENOMEM;
+			goto err_pdev;
+		}
+		pdev->dev.of_node = of_node_get(np);
+	}
+
+	regmap = devm_regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&pdev->dev, "regmap init failed\n");
+		ret = PTR_ERR(regmap);
+		goto err_regmap;
+	}
+
+	syscon->regmap = regmap;
+	syscon->np = np;
 
-	return (dev->of_node == dn) ? 1 : 0;
+	spin_lock(&syscon_list_slock);
+	list_add_tail(&syscon->list, &syscon_list);
+	spin_unlock(&syscon_list_slock);
+
+	return syscon;
+
+err_regmap:
+	if (!strcmp(pdev->name, "dummy-syscon")) {
+		of_node_put(np);
+		platform_device_put(pdev);
+	}
+err_pdev:
+	iounmap(base);
+err_map:
+	kfree(syscon);
+	return ERR_PTR(ret);
 }
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
-	struct syscon *syscon;
-	struct device *dev;
+	struct syscon *entry, *syscon = NULL;
 
-	dev = driver_find_device(&syscon_driver.driver, NULL, np,
-				 syscon_match_node);
-	if (!dev)
-		return ERR_PTR(-EPROBE_DEFER);
+	spin_lock(&syscon_list_slock);
 
-	syscon = dev_get_drvdata(dev);
+	list_for_each_entry(entry, &syscon_list, list)
+		if (entry->np == np) {
+			syscon = entry;
+			break;
+		}
+
+	spin_unlock(&syscon_list_slock);
+
+	if (!syscon)
+		syscon = of_syscon_register(np);
+
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
 
 	return syscon->regmap;
 }
@@ -110,17 +185,6 @@ 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 struct regmap_config syscon_regmap_config = {
-	.reg_bits = 32,
-	.val_bits = 32,
-	.reg_stride = 4,
-};
-
 static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -167,7 +231,6 @@ static struct platform_driver syscon_driver = {
 	.driver = {
 		.name = "syscon",
 		.owner = THIS_MODULE,
-		.of_match_table = of_syscon_match,
 	},
 	.probe		= syscon_probe,
 	.id_table	= syscon_ids,
-- 
1.7.9.5

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Dong Aisheng]

On Mon, Sep 22, 2014 at 10:10:07AM +0530, Pankaj Dubey wrote:
> Currently a syscon entity can be only registered directly through a
> platform device that binds to a dedicated syscon driver. However in
> certain use cases it is desirable to make a device used with another
> driver a syscon interface provider.
> 
> For example, certain SoCs (e.g. Exynos) contain system controller
> blocks which perform various functions such as power domain control,
> CPU power management, low power mode control, but in addition contain
> certain IP integration glue, such as various signal masks,
> coprocessor power control, etc. In such case, there is a need to have
> a dedicated driver for such system controller but also share registers
> with other drivers. The latter is where the syscon interface is helpful.
> 
> In case of DT based platforms, this patch decouples syscon object from
> syscon platform driver, and allows to create syscon objects first time
> when it is required by calling of syscon_regmap_lookup_by APIs and keep
> a list of such syscon objects along with syscon provider device_nodes
> and regmap handles.
> 
> For non-DT based platforms, this patch keeps syscon platform driver
> structure where is can be probed and such non-DT based drivers can use
> syscon_regmap_lookup_by_pdev API and get access to regmap handles.
> Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
> we can completly remove platform driver of syscon, and keep only helper
> functions to get regmap handles.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
> Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> ---
> Patch v4 and related discussions can be found here [1].
> 
> Changes since v4:
>  - Addressed Tomasz Figa's comments for v4.
>  - Added error handing in of_syscon_register function.
>  - Using devm_regmap_init_mmio instead of regmap_init_mmio.
> 
> Changes since v3:
>  - Addressed Arnd's comment for v2.
>  - Updated of_syscon_register for adding dev pointer in regmap_init_mmio.
>  - For early users created dummy platform device.
>    
> Changes since v2:
>  - Added back platform device support from syscon, with one change that
>    syscon will not be probed for DT based platform.
>  - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base
>    users of syscon will not be broken.
>  - Removed unwanted change in syscon.h.
>  - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and
>    Arnd Bergmann.
>  - Added Tested-by of Vivek Gautam for testing on Exynos platform.
> 
> Changes since v1:
>  - Removed of_syscon_unregister function.
>  - Modified of_syscon_register function and it will be used by syscon.c
>    to create syscon objects whenever required.
>  - Removed platform device support from syscon.
>  - Removed syscon_regmap_lookup_by_pdevname API support.
>  - As there are significant changes w.r.t patchset v1, I am taking over
>    author for this patchset from Tomasz Figa.
> 
> [1]: https://lkml.org/lkml/2014/9/19/267
> 
>  drivers/mfd/syscon.c |  107 +++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 85 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index ca15878..0b17f50 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -15,6 +15,7 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/list.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
> @@ -22,31 +23,105 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/slab.h>
>  
>  static struct platform_driver syscon_driver;
>  
> +static DEFINE_SPINLOCK(syscon_list_slock);
> +static LIST_HEAD(syscon_list);
> +
>  struct syscon {
> +	struct device_node *np;
>  	struct regmap *regmap;
> +	struct list_head list;
> +};
> +
> +static struct regmap_config syscon_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
>  };
>  
> -static int syscon_match_node(struct device *dev, void *data)
> +static struct syscon *of_syscon_register(struct device_node *np)
>  {
> -	struct device_node *dn = data;
> +	struct platform_device *pdev = NULL;
> +	struct syscon *syscon;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	int ret;
> +
> +	if (!of_device_is_compatible(np, "syscon"))
> +		return ERR_PTR(-EINVAL);
> +
> +	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> +	if (!syscon)
> +		return ERR_PTR(-ENOMEM);
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		ret = -ENOMEM;
> +		goto err_map;
> +	}
> +
> +	/* if device is already populated and available then use it */
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev) {
> +		/* for early users create dummy syscon device and use it */
> +		pdev = platform_device_alloc("dummy-syscon", -1);

Can we create specific devices for each syscon device?
That looks more reasonable to me.
Then we can dump registers in regmap debugfs more clearly, just like other devices.
And i think it's better to follow device tree way to generate devices from node.
If DT core find a device is already created, it can ignore that device to avoid
creating duplicated device during of_platform_populate.

Regards
Dong Aisheng

> +		if (!pdev) {
> +			ret = -ENOMEM;
> +			goto err_pdev;
> +		}
> +		pdev->dev.of_node = of_node_get(np);
> +	}
> +
> +	regmap = devm_regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&pdev->dev, "regmap init failed\n");
> +		ret = PTR_ERR(regmap);
> +		goto err_regmap;
> +	}
> +
> +	syscon->regmap = regmap;
> +	syscon->np = np;
>  
> -	return (dev->of_node == dn) ? 1 : 0;
> +	spin_lock(&syscon_list_slock);
> +	list_add_tail(&syscon->list, &syscon_list);
> +	spin_unlock(&syscon_list_slock);
> +
> +	return syscon;
> +
> +err_regmap:
> +	if (!strcmp(pdev->name, "dummy-syscon")) {
> +		of_node_put(np);
> +		platform_device_put(pdev);
> +	}
> +err_pdev:
> +	iounmap(base);
> +err_map:
> +	kfree(syscon);
> +	return ERR_PTR(ret);
>  }
>  
>  struct regmap *syscon_node_to_regmap(struct device_node *np)
>  {
> -	struct syscon *syscon;
> -	struct device *dev;
> +	struct syscon *entry, *syscon = NULL;
>  
> -	dev = driver_find_device(&syscon_driver.driver, NULL, np,
> -				 syscon_match_node);
> -	if (!dev)
> -		return ERR_PTR(-EPROBE_DEFER);
> +	spin_lock(&syscon_list_slock);
>  
> -	syscon = dev_get_drvdata(dev);
> +	list_for_each_entry(entry, &syscon_list, list)
> +		if (entry->np == np) {
> +			syscon = entry;
> +			break;
> +		}
> +
> +	spin_unlock(&syscon_list_slock);
> +
> +	if (!syscon)
> +		syscon = of_syscon_register(np);
> +
> +	if (IS_ERR(syscon))
> +		return ERR_CAST(syscon);
>  
>  	return syscon->regmap;
>  }
> @@ -110,17 +185,6 @@ 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 struct regmap_config syscon_regmap_config = {
> -	.reg_bits = 32,
> -	.val_bits = 32,
> -	.reg_stride = 4,
> -};
> -
>  static int syscon_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -167,7 +231,6 @@ static struct platform_driver syscon_driver = {
>  	.driver = {
>  		.name = "syscon",
>  		.owner = THIS_MODULE,
> -		.of_match_table = of_syscon_match,
>  	},
>  	.probe		= syscon_probe,
>  	.id_table	= syscon_ids,
> -- 
> 1.7.9.5
> 

RE: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Pankaj Dubey]

Hi Dong,

On Monday, September 22, 2014, Dong Aisheng wrote,
> On Mon, Sep 22, 2014 at 10:10:07AM +0530, Pankaj Dubey wrote:

[snip]

> > -static int syscon_match_node(struct device *dev, void *data)
> > +static struct syscon *of_syscon_register(struct device_node *np)
> >  {
> > -	struct device_node *dn = data;
> > +	struct platform_device *pdev = NULL;
> > +	struct syscon *syscon;
> > +	struct regmap *regmap;
> > +	void __iomem *base;
> > +	int ret;
> > +
> > +	if (!of_device_is_compatible(np, "syscon"))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > +	if (!syscon)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	base = of_iomap(np, 0);
> > +	if (!base) {
> > +		ret = -ENOMEM;
> > +		goto err_map;
> > +	}
> > +
> > +	/* if device is already populated and available then use it */
> > +	pdev = of_find_device_by_node(np);
> > +	if (!pdev) {
> > +		/* for early users create dummy syscon device and use it */
> > +		pdev = platform_device_alloc("dummy-syscon", -1);
> 
> Can we create specific devices for each syscon device?
> That looks more reasonable to me.
> Then we can dump registers in regmap debugfs more clearly, just like other
devices.
> And i think it's better to follow device tree way to generate devices from
node.
> If DT core find a device is already created, it can ignore that device to
avoid creating
> duplicated device during of_platform_populate.
> 

If you are referring to use "of_platform_device_create", then I am afraid
that it can't be used
in very early stage. For example, current patch I tested by calling
syscon_lookup_by_phandle from
"init_irq" hook of machine_desc, and it worked well, but If I use
"of_platform_device_create"
instead of dummy device, it fails to create pdev and this I verified on
Exynos platform. The reason
I selected "init_irq" is because this comes just before smp init and where
once we tried to get regmap handle,
but could not do so. 

Also if it was just a matter of creating platform_device at early stage then
early initialization of
syscon could have been solved till now.

So I would suggest if we really want this patch to cover early
initialization of syscon
(which is not it's main purpose) current patch is sufficient. 

@Arnd, since I have addressed crash issue as reported by Dong Aisheng, I
would like to take your ack,
but since there are some more code changes other than what you suggested I
request you to check this
and if you are ok, then I would like to your ack so that I can request to
maintainer for taking this patch.

Thanks,
Pankaj Dubey

> Regards
> Dong Aisheng
> 
> > +		if (!pdev) {
> > +			ret = -ENOMEM;
> > +			goto err_pdev;
> > +		}
> > +		pdev->dev.of_node = of_node_get(np);
> > +	}
> > +
> > +	regmap = devm_regmap_init_mmio(&pdev->dev, base,
> &syscon_regmap_config);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&pdev->dev, "regmap init failed\n");
> > +		ret = PTR_ERR(regmap);
> > +		goto err_regmap;
> > +	}
> > +
> > +	syscon->regmap = regmap;
> > +	syscon->np = np;
> >
> > -	return (dev->of_node == dn) ? 1 : 0;
> > +	spin_lock(&syscon_list_slock);
> > +	list_add_tail(&syscon->list, &syscon_list);
> > +	spin_unlock(&syscon_list_slock);
> > +
> > +	return syscon;
> > +
> > +err_regmap:
> > +	if (!strcmp(pdev->name, "dummy-syscon")) {
> > +		of_node_put(np);
> > +		platform_device_put(pdev);
> > +	}
> > +err_pdev:
> > +	iounmap(base);
> > +err_map:
> > +	kfree(syscon);
> > +	return ERR_PTR(ret);
> >  }
> >
> >  struct regmap *syscon_node_to_regmap(struct device_node *np)  {
> > -	struct syscon *syscon;
> > -	struct device *dev;
> > +	struct syscon *entry, *syscon = NULL;
> >
> > -	dev = driver_find_device(&syscon_driver.driver, NULL, np,
> > -				 syscon_match_node);
> > -	if (!dev)
> > -		return ERR_PTR(-EPROBE_DEFER);
> > +	spin_lock(&syscon_list_slock);
> >
> > -	syscon = dev_get_drvdata(dev);
> > +	list_for_each_entry(entry, &syscon_list, list)
> > +		if (entry->np == np) {
> > +			syscon = entry;
> > +			break;
> > +		}
> > +
> > +	spin_unlock(&syscon_list_slock);
> > +
> > +	if (!syscon)
> > +		syscon = of_syscon_register(np);
> > +
> > +	if (IS_ERR(syscon))
> > +		return ERR_CAST(syscon);
> >
> >  	return syscon->regmap;
> >  }
> > @@ -110,17 +185,6 @@ 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 struct regmap_config syscon_regmap_config = {
> > -	.reg_bits = 32,
> > -	.val_bits = 32,
> > -	.reg_stride = 4,
> > -};
> > -
> >  static int syscon_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> > @@ -167,7 +231,6 @@ static struct platform_driver syscon_driver = {
> >  	.driver = {
> >  		.name = "syscon",
> >  		.owner = THIS_MODULE,
> > -		.of_match_table = of_syscon_match,
> >  	},
> >  	.probe		= syscon_probe,
> >  	.id_table	= syscon_ids,
> > --
> > 1.7.9.5
> >

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Arnd Bergmann]

On Tuesday 23 September 2014 15:59:43 Pankaj Dubey wrote:
> > > -static int syscon_match_node(struct device *dev, void *data)
> > > +static struct syscon *of_syscon_register(struct device_node *np)
> > >  {
> > > -   struct device_node *dn = data;
> > > +   struct platform_device *pdev = NULL;
> > > +   struct syscon *syscon;
> > > +   struct regmap *regmap;
> > > +   void __iomem *base;
> > > +   int ret;
> > > +
> > > +   if (!of_device_is_compatible(np, "syscon"))
> > > +           return ERR_PTR(-EINVAL);
> > > +
> > > +   syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > > +   if (!syscon)
> > > +           return ERR_PTR(-ENOMEM);
> > > +
> > > +   base = of_iomap(np, 0);
> > > +   if (!base) {
> > > +           ret = -ENOMEM;
> > > +           goto err_map;
> > > +   }
> > > +
> > > +   /* if device is already populated and available then use it */
> > > +   pdev = of_find_device_by_node(np);
> > > +   if (!pdev) {
> > > +           /* for early users create dummy syscon device and use it */
> > > +           pdev = platform_device_alloc("dummy-syscon", -1);
> > 
> > Can we create specific devices for each syscon device?
> > That looks more reasonable to me.
> > Then we can dump registers in regmap debugfs more clearly, just like other
> devices.
> > And i think it's better to follow device tree way to generate devices from
> node.
> > If DT core find a device is already created, it can ignore that device to
> avoid creating
> > duplicated device during of_platform_populate.
> > 
> 
> If you are referring to use "of_platform_device_create", then I am afraid
> that it can't be used
> in very early stage. For example, current patch I tested by calling
> syscon_lookup_by_phandle from
> "init_irq" hook of machine_desc, and it worked well, but If I use
> "of_platform_device_create"
> instead of dummy device, it fails to create pdev and this I verified on
> Exynos platform. The reason
> I selected "init_irq" is because this comes just before smp init and where
> once we tried to get regmap handle,
> but could not do so. 

I don't remember noticing the of_find_device_by_node or platform_device_alloc
in earlier versions of this patch, but that could just be me failing to
read it right.

I think this should get removed: it would be much better to ensure that
the regmap code can work without a device pointer, which I thought it
did. We should probably also skip the creation of the debugfs directory
in that case.

> Also if it was just a matter of creating platform_device at early stage then
> early initialization of
> syscon could have been solved till now.
> 
> So I would suggest if we really want this patch to cover early
> initialization of syscon
> (which is not it's main purpose) current patch is sufficient. 
> 
> @Arnd, since I have addressed crash issue as reported by Dong Aisheng, I
> would like to take your ack,
> but since there are some more code changes other than what you suggested I
> request you to check this
> and if you are ok, then I would like to your ack so that I can request to
> maintainer for taking this patch.

I think we first need to resolve the question of the platform device.

	Arnd

RE: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Pankaj Dubey]

On Thursday, September 25, 2014 6:12 PM, Arnd Bergmann wrote,
> Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from
platform
> devices
> 
> On Tuesday 23 September 2014 15:59:43 Pankaj Dubey wrote:
> > > > -static int syscon_match_node(struct device *dev, void *data)
> > > > +static struct syscon *of_syscon_register(struct device_node *np)
> > > >  {
> > > > -   struct device_node *dn = data;
> > > > +   struct platform_device *pdev = NULL;
> > > > +   struct syscon *syscon;
> > > > +   struct regmap *regmap;
> > > > +   void __iomem *base;
> > > > +   int ret;
> > > > +
> > > > +   if (!of_device_is_compatible(np, "syscon"))
> > > > +           return ERR_PTR(-EINVAL);
> > > > +
> > > > +   syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > > > +   if (!syscon)
> > > > +           return ERR_PTR(-ENOMEM);
> > > > +
> > > > +   base = of_iomap(np, 0);
> > > > +   if (!base) {
> > > > +           ret = -ENOMEM;
> > > > +           goto err_map;
> > > > +   }
> > > > +
> > > > +   /* if device is already populated and available then use it */
> > > > +   pdev = of_find_device_by_node(np);
> > > > +   if (!pdev) {
> > > > +           /* for early users create dummy syscon device and use it
*/
> > > > +           pdev = platform_device_alloc("dummy-syscon", -1);
> > >
> > > Can we create specific devices for each syscon device?
> > > That looks more reasonable to me.
> > > Then we can dump registers in regmap debugfs more clearly, just like
> > > other
> > devices.
> > > And i think it's better to follow device tree way to generate
> > > devices from
> > node.
> > > If DT core find a device is already created, it can ignore that
> > > device to
> > avoid creating
> > > duplicated device during of_platform_populate.
> > >
> >
> > If you are referring to use "of_platform_device_create", then I am
> > afraid that it can't be used in very early stage. For example, current
> > patch I tested by calling syscon_lookup_by_phandle from "init_irq"
> > hook of machine_desc, and it worked well, but If I use
> > "of_platform_device_create"
> > instead of dummy device, it fails to create pdev and this I verified
> > on Exynos platform. The reason I selected "init_irq" is because this
> > comes just before smp init and where once we tried to get regmap
> > handle, but could not do so.
> 
> I don't remember noticing the of_find_device_by_node or
platform_device_alloc in
> earlier versions of this patch, but that could just be me failing to read
it right.
> 
Yes, till v2 of this patch set we do not used this and while calling
regmap_init_mmio, I used
NULL pointer while creating regmap handle.  But as you may be remember Dong
Aisheng reported crash
on latest Linus tree after applying this patch, and it caused because of
some recent changes in
regmap which has already landed in main tree, which makes passing of dev
pointer compulsory
while calling regmap_init_mmio. Please check following commits in current
tree

1) d647c19 regmap: add DT endianness binding support.
2) ba1b53f regmap: Fix DT endianess parsing logic.
3) 45e1a27 regmap: of_regmap_get_endian() cleanup

and following patch [1] for changing syscon binding from Xiubo Li

[1] [PATCH] mfd: syscon: binding: Add syscon endianness support
https://lkml.org/lkml/2014/9/18/67

So we discussed [2] and decided to use either actual device if it's
populated and available for use or
create either actual device or dummy device for using it during
regmap_init_mmio calls.

[2]: https://lkml.org/lkml/2014/9/18/35

So what I understood from these changes from Xiubo Li, they wanted to pass
endianness of regmap
handle to be passed from DT. 

So I am not sure if we can really make regmap code work without a device
pointer. 

> I think this should get removed: it would be much better to ensure that
the regmap
> code can work without a device pointer, which I thought it did. We should
probably
> also skip the creation of the debugfs directory in that case.
> 
> > Also if it was just a matter of creating platform_device at early
> > stage then early initialization of syscon could have been solved till
> > now.

As far as debugfs of regmap is concerned it can be solved as I replied to
Heiko in this thread [3]
if we are using device pointer while creating regmap handle. 
If we are not using device pointer while creating regmap handle it won't
cause any problem and 
there won't be any debugfs entry.

[3]: https://lkml.org/lkml/2014/9/26/20

> >
> > So I would suggest if we really want this patch to cover early
> > initialization of syscon (which is not it's main purpose) current
> > patch is sufficient.
> >
> > @Arnd, since I have addressed crash issue as reported by Dong Aisheng,
> > I would like to take your ack, but since there are some more code
> > changes other than what you suggested I request you to check this and
> > if you are ok, then I would like to your ack so that I can request to
> > maintainer for taking this patch.
> 
> I think we first need to resolve the question of the platform device.
> 

OK.

Thanks,
Pankaj Dubey

> 	Arnd

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Arnd Bergmann]

On Friday 26 September 2014 10:58:33 Pankaj Dubey wrote:
> On Thursday, September 25, 2014 6:12 PM, Arnd Bergmann wrote,
> > I don't remember noticing the of_find_device_by_node or
> > platform_device_alloc in
> > earlier versions of this patch, but that could just be me failing to read
> > it right.
> > 
> Yes, till v2 of this patch set we do not used this and while calling
> regmap_init_mmio, I used
> NULL pointer while creating regmap handle.  But as you may be remember Dong
> Aisheng reported crash
> on latest Linus tree after applying this patch, and it caused because of
> some recent changes in
> regmap which has already landed in main tree, which makes passing of dev
> pointer compulsory
> while calling regmap_init_mmio. Please check following commits in current
> tree
> 
> 1) d647c19 regmap: add DT endianness binding support.
> 2) ba1b53f regmap: Fix DT endianess parsing logic.
> 3) 45e1a27 regmap: of_regmap_get_endian() cleanup
> 
> and following patch [1] for changing syscon binding from Xiubo Li
> 
> [1] [PATCH] mfd: syscon: binding: Add syscon endianness support
> https://lkml.org/lkml/2014/9/18/67
> 
> So we discussed [2] and decided to use either actual device if it's
> populated and available for use or
> create either actual device or dummy device for using it during
> regmap_init_mmio calls.
> 
> [2]: https://lkml.org/lkml/2014/9/18/35
> 
> So what I understood from these changes from Xiubo Li, they wanted to pass
> endianness of regmap
> handle to be passed from DT. 
> 
> So I am not sure if we can really make regmap code work without a device
> pointer. 

Why can't we just have both a dev pointer and an explicit of_node pointer
that can be used in the absence of the device?

I think it's dangerous to have a platform device that is allocated but
not registered as you do in your driver at the moment. We can't register
it for early callers either, and if we found a way to do so, we'd end
up with multiple platform devices that contain the same of_node, which
would then cause other problems, such as binding both devices to the
same driver.

> > I think this should get removed: it would be much better to ensure that
> the regmap
> > code can work without a device pointer, which I thought it did. We should
> probably
> > also skip the creation of the debugfs directory in that case.
> > 
> > > Also if it was just a matter of creating platform_device at early
> > > stage then early initialization of syscon could have been solved till
> > > now.
> 
> As far as debugfs of regmap is concerned it can be solved as I replied to
> Heiko in this thread [3]
> if we are using device pointer while creating regmap handle. 
> If we are not using device pointer while creating regmap handle it won't
> cause any problem and 
> there won't be any debugfs entry.
> 
> [3]: https://lkml.org/lkml/2014/9/26/20

Right, this works, although my earlier thoughts were that we would
just never have a device for a syscon any more. We could have a
different representation for syscon regmaps in debugfs though, one
that is separate from the devices and that is based on the just the
of_node pointer. syscon is special enough that I think the regmap
infrastructure should be aware of what is a syscon regmap as opposed
to one from a device.

	Arnd

RE: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Li.Xiubo]

Hi,

[...]
> +static struct regmap_config syscon_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
>  };
> 
> -static int syscon_match_node(struct device *dev, void *data)
> +static struct syscon *of_syscon_register(struct device_node *np)
>  {
> -	struct device_node *dn = data;
> +	struct platform_device *pdev = NULL;

struct platform_device *pdev; ?

Thanks,

BRs
Xiubo

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Joachim Eastwood]

On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> wrote:
> Currently a syscon entity can be only registered directly through a
> platform device that binds to a dedicated syscon driver. However in
> certain use cases it is desirable to make a device used with another
> driver a syscon interface provider.
>
> For example, certain SoCs (e.g. Exynos) contain system controller
> blocks which perform various functions such as power domain control,
> CPU power management, low power mode control, but in addition contain
> certain IP integration glue, such as various signal masks,
> coprocessor power control, etc. In such case, there is a need to have
> a dedicated driver for such system controller but also share registers
> with other drivers. The latter is where the syscon interface is helpful.
>
> In case of DT based platforms, this patch decouples syscon object from
> syscon platform driver, and allows to create syscon objects first time
> when it is required by calling of syscon_regmap_lookup_by APIs and keep
> a list of such syscon objects along with syscon provider device_nodes
> and regmap handles.
>
> For non-DT based platforms, this patch keeps syscon platform driver
> structure where is can be probed and such non-DT based drivers can use
> syscon_regmap_lookup_by_pdev API and get access to regmap handles.
> Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
> we can completly remove platform driver of syscon, and keep only helper
> functions to get regmap handles.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
> Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>

Hi Pankaj,

I wrote a clk driver using syscon and your patch. clk driver uses
CLK_OF_DECLARE, btw.

It works but I get a '(null): Failed to create debugfs directory'
message in the boot log.

Tested-by: Joachim Eastwood <manabian@gmail.com>

regards,
Joachim Eastwood

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Heiko Stübner]

Hi Pankaj, Joachim,

Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood:
> On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> wrote:
> > Currently a syscon entity can be only registered directly through a
> > platform device that binds to a dedicated syscon driver. However in
> > certain use cases it is desirable to make a device used with another
> > driver a syscon interface provider.
> > 
> > For example, certain SoCs (e.g. Exynos) contain system controller
> > blocks which perform various functions such as power domain control,
> > CPU power management, low power mode control, but in addition contain
> > certain IP integration glue, such as various signal masks,
> > coprocessor power control, etc. In such case, there is a need to have
> > a dedicated driver for such system controller but also share registers
> > with other drivers. The latter is where the syscon interface is helpful.
> > 
> > In case of DT based platforms, this patch decouples syscon object from
> > syscon platform driver, and allows to create syscon objects first time
> > when it is required by calling of syscon_regmap_lookup_by APIs and keep
> > a list of such syscon objects along with syscon provider device_nodes
> > and regmap handles.
> > 
> > For non-DT based platforms, this patch keeps syscon platform driver
> > structure where is can be probed and such non-DT based drivers can use
> > syscon_regmap_lookup_by_pdev API and get access to regmap handles.
> > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
> > we can completly remove platform driver of syscon, and keep only helper
> > functions to get regmap handles.
> > 
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
> > Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
> > Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> 
> I wrote a clk driver using syscon and your patch. clk driver uses
> CLK_OF_DECLARE, btw.
> 
> It works but I get a '(null): Failed to create debugfs directory'
> message in the boot log.
> 
> Tested-by: Joachim Eastwood <manabian@gmail.com>

on Rockchip platforms this syscon support also helps quite a bit, as the
pll lock-status is sitting in an external syscon register, so setting target
pll-rates through assigned-clocks is not easily doable without it.
Therefore I'm very much looking forward to this.


Similar to Joachim I get an error about debugfs from regmap, which seems
to be caused by
	name = dev_name(map->dev);
returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an "early"
syscon.

[...]
__set_clk_rates: setting cpll from 384000000 to 891000000
rockchip_rk3066_pll_set_rate: trying to get grf
rockchip_rk3066_pll_set_rate: changing pll_cpll from 384000000 to 891000000 with a parent rate of 24000000
__set_clk_rates: cpll is now 891000000
Architected cp15 timer(s) running at 24.00MHz (virt).
[...]
regulator-dummy: no parameters
NET: Registered protocol family 16
DMA: preallocated 256 KiB pool for atomic coherent allocations
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = c0004000
[00000000] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc1+ #1184
task: ee069b40 ti: ee06a000 task.ti: ee06a000
PC is at strlen+0xc/0x20
LR is at __create_file+0x6c/0x1d0
pc : [<c01a02f8>]    lr : [<c01725f0>]    psr: 60000153
sp : ee06bea8  ip : 0000000e  fp : 00000000
r10: ee06a000  r9 : 00000000  r8 : 00000000
r7 : c07ecb20  r6 : edc02cc0  r5 : 000041ed  r4 : 00000000
r3 : 00000001  r2 : 00000000  r1 : a0000153  r0 : 00000000
Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 0000406a  DAC: 00000015
Process swapper/0 (pid: 1, stack limit = 0xee06a240)
Stack: (0xee06bea8 to 0xee06c000)
bea0:                   00000000 000041ed ee002400 c07efc60 00000000 c05a9e50
bec0: c058c510 c01727c8 00000000 edc02cc0 ee0024b0 c021d410 ee002400 00000000
bee0: ee02ff40 c0793494 c07ab080 c021d6b4 00000000 ee11bbc0 c0782c58 c058c518
bf00: 00000000 c0008970 ee0f7f00 c00f9f54 ee0f7f00 ee0f7c80 ee0f7c00 c03fddc4
bf20: c07cda04 00000000 c054c8a4 c00fa0e0 c0575514 ef7fccc5 00000000 c0034fd0
bf40: 00000000 00000000 c054c8a4 c054bcd4 000000bd 00000002 c0786394 00000002
bf60: c059e840 c07ab080 c05a9e50 000000bd 00000000 00000000 00000000 c0575c94
bf80: 00000002 00000002 c0575514 ee06a000 00000000 c03f38f8 00000000 00000000
bfa0: 00000000 c03f3900 00000000 c000e7f8 00000000 00000000 00000000 00000000
bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 ffffffff ffffffff
[<c01a02f8>] (strlen) from [<c01725f0>] (__create_file+0x6c/0x1d0)
[<c01725f0>] (__create_file) from [<c01727c8>] (debugfs_create_dir+0x18/0x1c)
[<c01727c8>] (debugfs_create_dir) from [<c021d410>] (regmap_debugfs_init+0xd0/0x254)
[<c021d410>] (regmap_debugfs_init) from [<c021d6b4>] (regmap_debugfs_initcall+0x5c/0xbc)
[<c021d6b4>] (regmap_debugfs_initcall) from [<c058c518>] (regmap_initcall+0x8/0x10)
[<c058c518>] (regmap_initcall) from [<c0008970>] (do_one_initcall+0x110/0x1bc)
[<c0008970>] (do_one_initcall) from [<c0575c94>] (kernel_init_freeable+0xfc/0x1c8)
[<c0575c94>] (kernel_init_freeable) from [<c03f3900>] (kernel_init+0x8/0xe4)
[<c03f3900>] (kernel_init) from [<c000e7f8>] (ret_from_fork+0x14/0x3c)
Code: c040e4cc e1a03000 e1a02003 e2833001 (e5d21000) 
---[ end trace fa51dc7fc31bd989 ]---

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Heiko Stübner]

Am Mittwoch, 24. September 2014, 20:35:10 schrieb Heiko Stübner:
> Hi Pankaj, Joachim,
> 
> Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood:
> > On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> wrote:
> > > Currently a syscon entity can be only registered directly through a
> > > platform device that binds to a dedicated syscon driver. However in
> > > certain use cases it is desirable to make a device used with another
> > > driver a syscon interface provider.
> > > 
> > > For example, certain SoCs (e.g. Exynos) contain system controller
> > > blocks which perform various functions such as power domain control,
> > > CPU power management, low power mode control, but in addition contain
> > > certain IP integration glue, such as various signal masks,
> > > coprocessor power control, etc. In such case, there is a need to have
> > > a dedicated driver for such system controller but also share registers
> > > with other drivers. The latter is where the syscon interface is helpful.
> > > 
> > > In case of DT based platforms, this patch decouples syscon object from
> > > syscon platform driver, and allows to create syscon objects first time
> > > when it is required by calling of syscon_regmap_lookup_by APIs and keep
> > > a list of such syscon objects along with syscon provider device_nodes
> > > and regmap handles.
> > > 
> > > For non-DT based platforms, this patch keeps syscon platform driver
> > > structure where is can be probed and such non-DT based drivers can use
> > > syscon_regmap_lookup_by_pdev API and get access to regmap handles.
> > > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
> > > we can completly remove platform driver of syscon, and keep only helper
> > > functions to get regmap handles.
> > > 
> > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
> > > Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > 
> > I wrote a clk driver using syscon and your patch. clk driver uses
> > CLK_OF_DECLARE, btw.
> > 
> > It works but I get a '(null): Failed to create debugfs directory'
> > message in the boot log.
> > 
> > Tested-by: Joachim Eastwood <manabian@gmail.com>
> 
> on Rockchip platforms this syscon support also helps quite a bit, as the
> pll lock-status is sitting in an external syscon register, so setting target
> pll-rates through assigned-clocks is not easily doable without it.
> Therefore I'm very much looking forward to this.
> 
> 
> Similar to Joachim I get an error about debugfs from regmap, which seems
> to be caused by
> 	name = dev_name(map->dev);
> returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an
> "early" syscon.

It looks like of_device_make_bus_id would be able to do the necessary steps
to populate the dev_name seemingly correctly.

With the diff below I now get a syscon that can init clocks and also a
sane regmap debugfs init:

/debug/regmap # ls -la
total 0
drwxr-xr-x    5 0        0                0 Jan  1  1970 .
drwx------   19 0        0                0 Jan  1  1970 ..
drwxr-xr-x    2 0        0                0 Jan  1  1970 0-001b
drwxr-xr-x    2 0        0                0 Jan  1  1970 ff730000.power-management
drwxr-xr-x    2 0        0                0 Jan  1  1970 ff770000.syscon


But of course I don't know enough about device-internals to determine if
this is an insane solution or not :-)


Heiko


diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 8ebc1c6..3734434 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -73,6 +73,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
                        goto err_pdev;
                }
                pdev->dev.of_node = of_node_get(np);
+               of_device_make_bus_id(&pdev->dev);
        }
 
        regmap = devm_regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config);

RE: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Pankaj Dubey]

Hi Heiko and Joachim,

> -----Original Message-----
> From: Heiko Stübner [mailto:heiko@sntech.de]
> Sent: Thursday, September 25, 2014 5:52 PM
> To: Pankaj Dubey
> Cc: Joachim Eastwood; linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org; linux-kernel@vger.kernel.org; kgene.kim@samsung.com;
> Russell King - ARM Linux; Arnd Bergmann; naushad@samsung.com;
> b29396@freescale.com; tomasz.figa@gmail.com; joshi@samsung.com;
> thomas.ab@samsung.com; Li.Xiubo@freescale.com; vikas.sajjan@samsung.com;
> chow.kim@samsung.com; lee.jones@linaro.org; dianders@chromium.org
> Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from
platform
> devices
> 
> Am Mittwoch, 24. September 2014, 20:35:10 schrieb Heiko Stübner:
> > Hi Pankaj, Joachim,
> >
> > Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood:
> > > On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com>
> wrote:
> > > > Currently a syscon entity can be only registered directly through
> > > > a platform device that binds to a dedicated syscon driver. However
> > > > in certain use cases it is desirable to make a device used with
> > > > another driver a syscon interface provider.
> > > >
> > > > For example, certain SoCs (e.g. Exynos) contain system controller
> > > > blocks which perform various functions such as power domain
> > > > control, CPU power management, low power mode control, but in
> > > > addition contain certain IP integration glue, such as various
> > > > signal masks, coprocessor power control, etc. In such case, there
> > > > is a need to have a dedicated driver for such system controller
> > > > but also share registers with other drivers. The latter is where the
syscon
> interface is helpful.
> > > >
> > > > In case of DT based platforms, this patch decouples syscon object
> > > > from syscon platform driver, and allows to create syscon objects
> > > > first time when it is required by calling of
> > > > syscon_regmap_lookup_by APIs and keep a list of such syscon
> > > > objects along with syscon provider device_nodes and regmap handles.
> > > >
> > > > For non-DT based platforms, this patch keeps syscon platform
> > > > driver structure where is can be probed and such non-DT based
> > > > drivers can use syscon_regmap_lookup_by_pdev API and get access to
> regmap handles.
> > > > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT
> > > > based, we can completly remove platform driver of syscon, and keep
> > > > only helper functions to get regmap handles.
> > > >
> > > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
> > > > Tested-by: Javier Martinez Canillas
> > > > <javier.martinez@collabora.co.uk>
> > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > >
> > > I wrote a clk driver using syscon and your patch. clk driver uses
> > > CLK_OF_DECLARE, btw.
> > >
> > > It works but I get a '(null): Failed to create debugfs directory'
> > > message in the boot log.
> > >
> > > Tested-by: Joachim Eastwood <manabian@gmail.com>
> >
> > on Rockchip platforms this syscon support also helps quite a bit, as
> > the pll lock-status is sitting in an external syscon register, so
> > setting target pll-rates through assigned-clocks is not easily doable
without it.
> > Therefore I'm very much looking forward to this.
> >
> >
> > Similar to Joachim I get an error about debugfs from regmap, which
> > seems to be caused by
> > 	name = dev_name(map->dev);
> > returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an
> > "early" syscon.
> 
> It looks like of_device_make_bus_id would be able to do the necessary
steps to
> populate the dev_name seemingly correctly.
> 
> With the diff below I now get a syscon that can init clocks and also a
sane regmap
> debugfs init:
> 
> /debug/regmap # ls -la
> total 0
> drwxr-xr-x    5 0        0                0 Jan  1  1970 .
> drwx------   19 0        0                0 Jan  1  1970 ..
> drwxr-xr-x    2 0        0                0 Jan  1  1970 0-001b
> drwxr-xr-x    2 0        0                0 Jan  1  1970
ff730000.power-management
> drwxr-xr-x    2 0        0                0 Jan  1  1970 ff770000.syscon
> 
> 
> But of course I don't know enough about device-internals to determine if
this is an
> insane solution or not :-)
> 

Thanks Heiko for figuring out issue and proposed solution.

As you and Joachim pointed out that current patch failed to create regmap
debugfs entry,
I also investigated and found that it fails to create regmap debugfs entry
either you call it
early (from init_irq or clk_init function) or you call it in later stage
before actual device is
populated (from init_machine before of_platform_populate_device).

One point is regmap debugfs code should have handled it gracefully instead
of kernel panic,
so looks like it needs some fix in that part of code.

I tried Heiko's suggested solution of calling "of_device_make_bus_id" after
platform_device_alloc
and it worked well and I tested it from init_irq as well as clk_init, which
happens at very early stage.
Maybe Joachim can also try if it's working for him.

Only concerns for this approach: Is it proper way of doing this?

In my opinion it could be, if we are not getting any other approach of
handling early syscon.

I also tried to get any other solution for handling debugfs entry, and found
one more solution.
But it will work only for late users of syscon: 

pdev = platform_device_alloc("dummy-syscon", -1);
ret = platform_device_add(pdev);

It can solve issue of regmap debugfs for late users, i.e. if we try to use
syscon_lookup_by APIs not before
init_machine. But this will not work for early users of syscon, as I can see
that "platform_device_add"
fails (kernel panic) if we call it from init_irq or clk_init.

Meanwhile I have posted a fix [1] for this so that if someone calls
platform_device_add at very early
stage at least it should not panic and kernel should handle it gracefully. 

[1]: https://lkml.org/lkml/2014/9/24/348

Also as I mentioned earlier, providing feature of early syscon availability
was not the main objective of this
patch, so just to make sure that this should not be a blocking factor for
current patch I would like to split
patch in two steps to address to different issues:

1: First patch will cover existing users and allow any such users to create
actual platform device and register
their own platform driver, instead of syscon getting probed first.
I will make sure that it should not break existing users or any existing
functionality (including debugfs).

2: Second on top of this patch we can have discussion how to go for early
syscon users.

Thanks,
Pankaj Dubey
> 
> Heiko
> 
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index
8ebc1c6..3734434
> 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -73,6 +73,7 @@ static struct syscon *of_syscon_register(struct
device_node
> *np)
>                         goto err_pdev;
>                 }
>                 pdev->dev.of_node = of_node_get(np);
> +               of_device_make_bus_id(&pdev->dev);
>         }
> 
>         regmap = devm_regmap_init_mmio(&pdev->dev, base,
> &syscon_regmap_config);

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Joachim Eastwood]

On 26 September 2014 06:56, Pankaj Dubey <pankaj.dubey@samsung.com> wrote:
> Hi Heiko and Joachim,
>
>> -----Original Message-----
>> From: Heiko Stübner [mailto:heiko@sntech.de]
>> Sent: Thursday, September 25, 2014 5:52 PM
>> To: Pankaj Dubey
>> Cc: Joachim Eastwood; linux-arm-kernel@lists.infradead.org; linux-samsung-
>> soc@vger.kernel.org; linux-kernel@vger.kernel.org; kgene.kim@samsung.com;
>> Russell King - ARM Linux; Arnd Bergmann; naushad@samsung.com;
>> b29396@freescale.com; tomasz.figa@gmail.com; joshi@samsung.com;
>> thomas.ab@samsung.com; Li.Xiubo@freescale.com; vikas.sajjan@samsung.com;
>> chow.kim@samsung.com; lee.jones@linaro.org; dianders@chromium.org
>> Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from
> platform
>> devices
>>
>> Am Mittwoch, 24. September 2014, 20:35:10 schrieb Heiko Stübner:
>> > Hi Pankaj, Joachim,
>> >
>> > Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood:
>> > > On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com>
>> wrote:
>> > > > Currently a syscon entity can be only registered directly through
>> > > > a platform device that binds to a dedicated syscon driver. However
>> > > > in certain use cases it is desirable to make a device used with
>> > > > another driver a syscon interface provider.
>> > > >
>> > > > For example, certain SoCs (e.g. Exynos) contain system controller
>> > > > blocks which perform various functions such as power domain
>> > > > control, CPU power management, low power mode control, but in
>> > > > addition contain certain IP integration glue, such as various
>> > > > signal masks, coprocessor power control, etc. In such case, there
>> > > > is a need to have a dedicated driver for such system controller
>> > > > but also share registers with other drivers. The latter is where the
> syscon
>> interface is helpful.
>> > > >
>> > > > In case of DT based platforms, this patch decouples syscon object
>> > > > from syscon platform driver, and allows to create syscon objects
>> > > > first time when it is required by calling of
>> > > > syscon_regmap_lookup_by APIs and keep a list of such syscon
>> > > > objects along with syscon provider device_nodes and regmap handles.
>> > > >
>> > > > For non-DT based platforms, this patch keeps syscon platform
>> > > > driver structure where is can be probed and such non-DT based
>> > > > drivers can use syscon_regmap_lookup_by_pdev API and get access to
>> regmap handles.
>> > > > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT
>> > > > based, we can completly remove platform driver of syscon, and keep
>> > > > only helper functions to get regmap handles.
>> > > >
>> > > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> > > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
>> > > > Tested-by: Vivek Gautam <gautam.vivek@samsung.com>
>> > > > Tested-by: Javier Martinez Canillas
>> > > > <javier.martinez@collabora.co.uk>
>> > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>> > >
>> > > I wrote a clk driver using syscon and your patch. clk driver uses
>> > > CLK_OF_DECLARE, btw.
>> > >
>> > > It works but I get a '(null): Failed to create debugfs directory'
>> > > message in the boot log.
>> > >
>> > > Tested-by: Joachim Eastwood <manabian@gmail.com>
>> >
>> > on Rockchip platforms this syscon support also helps quite a bit, as
>> > the pll lock-status is sitting in an external syscon register, so
>> > setting target pll-rates through assigned-clocks is not easily doable
> without it.
>> > Therefore I'm very much looking forward to this.
>> >
>> >
>> > Similar to Joachim I get an error about debugfs from regmap, which
>> > seems to be caused by
>> >     name = dev_name(map->dev);
>> > returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an
>> > "early" syscon.
>>
>> It looks like of_device_make_bus_id would be able to do the necessary
> steps to
>> populate the dev_name seemingly correctly.
>>
>> With the diff below I now get a syscon that can init clocks and also a
> sane regmap
>> debugfs init:
>>
>> /debug/regmap # ls -la
>> total 0
>> drwxr-xr-x    5 0        0                0 Jan  1  1970 .
>> drwx------   19 0        0                0 Jan  1  1970 ..
>> drwxr-xr-x    2 0        0                0 Jan  1  1970 0-001b
>> drwxr-xr-x    2 0        0                0 Jan  1  1970
> ff730000.power-management
>> drwxr-xr-x    2 0        0                0 Jan  1  1970 ff770000.syscon
>>
>>
>> But of course I don't know enough about device-internals to determine if
> this is an
>> insane solution or not :-)
>>
>
> Thanks Heiko for figuring out issue and proposed solution.
>
> As you and Joachim pointed out that current patch failed to create regmap
> debugfs entry,
> I also investigated and found that it fails to create regmap debugfs entry
> either you call it
> early (from init_irq or clk_init function) or you call it in later stage
> before actual device is
> populated (from init_machine before of_platform_populate_device).
>
> One point is regmap debugfs code should have handled it gracefully instead
> of kernel panic,
> so looks like it needs some fix in that part of code.

Just for the records. My kernel didn't panic.
Don't know why it behaves different from Heiko's kernel but I was able
to boot into user space with your patch.

I wouldn't have given my 'Tested-by' if didn't boot properly.

I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw.

regards
Joachim Eastwood

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Arnd Bergmann]

On Friday 26 September 2014 07:34:12 Joachim Eastwood wrote:
> I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw.
> 

Sorry for drifting off-topic, but this is very interesting to me. Can you
say which one you are working on and what your timeline is for submitting
it upstream?

I generally like to know about new platforms early, and sometimes we get
multiple people working on the same one.

	Arnd

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Joachim Eastwood]

On 26 September 2014 09:16, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 26 September 2014 07:34:12 Joachim Eastwood wrote:
>> I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw.
>>
>
> Sorry for drifting off-topic, but this is very interesting to me. Can you
> say which one you are working on and what your timeline is for submitting
> it upstream?

It's NXP LPC18xx/43xx which is Cortex-M3/M4.

3.19 or 3.20 might be target.

Right now everything is in a github repository here:
https://github.com/manabian/linux-lpc

Most stuff are working now and I am in the process of clean it up and
adding documentation.

> I generally like to know about new platforms early, and sometimes we get
> multiple people working on the same one.

I see.

regards,
Joachim Eastwood

Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Arnd Bergmann]

On Friday 26 September 2014 09:48:24 Joachim Eastwood wrote:
> On 26 September 2014 09:16, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 26 September 2014 07:34:12 Joachim Eastwood wrote:
> >> I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw.
> >>
> >
> > Sorry for drifting off-topic, but this is very interesting to me. Can you
> > say which one you are working on and what your timeline is for submitting
> > it upstream?
> 
> It's NXP LPC18xx/43xx which is Cortex-M3/M4.
> 
> 3.19 or 3.20 might be target.

Ah, very nice!

> Right now everything is in a github repository here:
> https://github.com/manabian/linux-lpc
> 
> Most stuff are working now and I am in the process of clean it up and
> adding documentation.

Ok, looks like you are making good progress. I noticed three high-level
issues that you may want to address:

- the watchdog driver should use the generic watchdog framework rather
  than registering a misc device.
- the ehci glue can probably go away if you make very small changes
  to the generic ehci platform driver
- I don't like the way that the stmmac glue drivers are added, I thought
  we had fixed this before but I think I need to dig up old emails.
  The driver should really be a loadable module that hooks calls into
  the common code rather than being linked into one module.

I also have a plan for doing multiplatform builds of nommu kernels, 
for build testing mostly, I wouldn't expect you to run that configuration.
No need for you to address that yourself though, we'll get there.

	Arnd