From: Pankaj Dubey <pankaj.dubey@samsung.com> To: "'Heiko Stübner'" <heiko@sntech.de> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, arnd@arndb.de, b29396@freescale.com, lee.jones@linaro.org, boris.brezillon@free-electrons.com, tomasz.figa@gmail.com, linux@arm.linux.org.uk, vikas.sajjan@samsung.com, naushad@samsung.com, thomas.ab@samsung.com, kgene.kim@samsung.com, Li.Xiubo@freescale.com, geert+renesas@glider.be, swarren@nvidia.com, linux-rockchip@lists.infradead.org Subject: RE: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices Date: Tue, 30 Sep 2014 09:33:38 +0530 [thread overview] Message-ID: <000d01cfdc63$99576bb0$cc064310$@samsung.com> (raw) In-Reply-To: <1617081.XFKIz4hM4O@phil> Hi, On Monday, September 29, 2014 9:38 PM, Heiko Stübner wrote, > Am Montag, 29. September 2014, 14:17:38 schrieb 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 completely 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> > > --- > > On Rockchip boards during core clock init (aka before timers) > Tested-by: Heiko Stuebner <heiko@sntech.de> > Thanks for testing. > Except one issue described inline below > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > > And I'm really looking forward to having this in the kernel :-) > > Thanks for working on this > Heiko > > [snip] > > > > drivers/mfd/syscon.c | 106 > > +++++++++++++++++++++++++++++++++++++++----------- 1 file > changed, 84 > > insertions(+), 22 deletions(-) > > > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index > > ca15878..00a8410 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,104 @@ > > #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 syscon *syscon; > > + struct regmap *regmap; > > + void __iomem *base; > > + int ret; > > + enum regmap_endian endian = REGMAP_ENDIAN_DEFAULT; > > + > > + 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; > > + } > > + > > + /* Parse the device's DT node for an endianness specification */ > > + if (of_property_read_bool(np, "big-endian")) > > + endian = REGMAP_ENDIAN_BIG; > > + else if (of_property_read_bool(np, "little-endian")) > > + endian = REGMAP_ENDIAN_LITTLE; > > + > > + /* If the endianness was specified in DT, use that */ > > + if (endian != REGMAP_ENDIAN_DEFAULT) > > + syscon_regmap_config.val_format_endian = endian; > > + > > + regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > > + if (IS_ERR(regmap)) { > > + pr_err("regmap init failed\n"); > > + ret = PTR_ERR(regmap); > > + goto err_regmap; > > + } > > + > > + syscon->regmap = regmap; > > + syscon->np = np; > > + > > + spin_lock(&syscon_list_slock); > > + list_add_tail(&syscon->list, &syscon_list); > > + spin_unlock(&syscon_list_slock); > > > > - return (dev->of_node == dn) ? 1 : 0; > > + /* Change back endianness of syscon_regmap_config. > > + * As this is static config in this file and in one system we may > > + * have more than one syscon > > + */ > > + syscon_regmap_config.val_format_endian = > REGMAP_ENDIAN_DEFAULT; > > This should also be done in the error case. Currently when you goto err_regmap the > overridden value will be left in the struct. > Thanks, will handle this in error condition also. > While on this, is there a concurrency issue here, aka of_syscon_register could be > called in parallel and what happens with syscon_regmap_config.val_format_endian > then? I can think of two approaches to solve this. 1: Updating syscon_regmap_config, under spin_lock "syscon_list_slock". 2: Creation of local copy of syscon_regmap_config in "of_syscon_register" and using it. In this case changing back of endianness in syscon_regmap_config, will not be needed and code will be a bit cleaner. I would prefer second one, what is your opinion? Thanks, Pankaj Dubey > > > > + > > + return syscon; > > + > > +err_regmap: > > + 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 +184,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 +230,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,
WARNING: multiple messages have this Message-ID (diff)
From: pankaj.dubey@samsung.com (Pankaj Dubey) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices Date: Tue, 30 Sep 2014 09:33:38 +0530 [thread overview] Message-ID: <000d01cfdc63$99576bb0$cc064310$@samsung.com> (raw) In-Reply-To: <1617081.XFKIz4hM4O@phil> Hi, On Monday, September 29, 2014 9:38 PM, Heiko St?bner wrote, > Am Montag, 29. September 2014, 14:17:38 schrieb 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 completely 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> > > --- > > On Rockchip boards during core clock init (aka before timers) > Tested-by: Heiko Stuebner <heiko@sntech.de> > Thanks for testing. > Except one issue described inline below > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > > And I'm really looking forward to having this in the kernel :-) > > Thanks for working on this > Heiko > > [snip] > > > > drivers/mfd/syscon.c | 106 > > +++++++++++++++++++++++++++++++++++++++----------- 1 file > changed, 84 > > insertions(+), 22 deletions(-) > > > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index > > ca15878..00a8410 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,104 @@ > > #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 syscon *syscon; > > + struct regmap *regmap; > > + void __iomem *base; > > + int ret; > > + enum regmap_endian endian = REGMAP_ENDIAN_DEFAULT; > > + > > + 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; > > + } > > + > > + /* Parse the device's DT node for an endianness specification */ > > + if (of_property_read_bool(np, "big-endian")) > > + endian = REGMAP_ENDIAN_BIG; > > + else if (of_property_read_bool(np, "little-endian")) > > + endian = REGMAP_ENDIAN_LITTLE; > > + > > + /* If the endianness was specified in DT, use that */ > > + if (endian != REGMAP_ENDIAN_DEFAULT) > > + syscon_regmap_config.val_format_endian = endian; > > + > > + regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > > + if (IS_ERR(regmap)) { > > + pr_err("regmap init failed\n"); > > + ret = PTR_ERR(regmap); > > + goto err_regmap; > > + } > > + > > + syscon->regmap = regmap; > > + syscon->np = np; > > + > > + spin_lock(&syscon_list_slock); > > + list_add_tail(&syscon->list, &syscon_list); > > + spin_unlock(&syscon_list_slock); > > > > - return (dev->of_node == dn) ? 1 : 0; > > + /* Change back endianness of syscon_regmap_config. > > + * As this is static config in this file and in one system we may > > + * have more than one syscon > > + */ > > + syscon_regmap_config.val_format_endian = > REGMAP_ENDIAN_DEFAULT; > > This should also be done in the error case. Currently when you goto err_regmap the > overridden value will be left in the struct. > Thanks, will handle this in error condition also. > While on this, is there a concurrency issue here, aka of_syscon_register could be > called in parallel and what happens with syscon_regmap_config.val_format_endian > then? I can think of two approaches to solve this. 1: Updating syscon_regmap_config, under spin_lock "syscon_list_slock". 2: Creation of local copy of syscon_regmap_config in "of_syscon_register" and using it. In this case changing back of endianness in syscon_regmap_config, will not be needed and code will be a bit cleaner. I would prefer second one, what is your opinion? Thanks, Pankaj Dubey > > > > + > > + return syscon; > > + > > +err_regmap: > > + 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 +184,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 +230,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,
next prev parent reply other threads:[~2014-09-30 4:04 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-29 8:47 [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices Pankaj Dubey 2014-09-29 8:47 ` Pankaj Dubey 2014-09-29 9:46 ` Arnd Bergmann 2014-09-29 9:46 ` Arnd Bergmann 2014-09-29 16:07 ` Heiko Stübner 2014-09-29 16:07 ` Heiko Stübner 2014-09-30 4:03 ` Pankaj Dubey [this message] 2014-09-30 4:03 ` Pankaj Dubey 2014-09-30 7:27 ` Boris Brezillon 2014-09-30 7:27 ` Boris Brezillon 2014-09-30 7:32 ` Heiko Stübner 2014-09-30 7:32 ` Heiko Stübner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='000d01cfdc63$99576bb0$cc064310$@samsung.com' \ --to=pankaj.dubey@samsung.com \ --cc=Li.Xiubo@freescale.com \ --cc=arnd@arndb.de \ --cc=b29396@freescale.com \ --cc=boris.brezillon@free-electrons.com \ --cc=geert+renesas@glider.be \ --cc=heiko@sntech.de \ --cc=kgene.kim@samsung.com \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=naushad@samsung.com \ --cc=swarren@nvidia.com \ --cc=thomas.ab@samsung.com \ --cc=tomasz.figa@gmail.com \ --cc=vikas.sajjan@samsung.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.