| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140930092727.12c34f05@bbrezillon>
Date: Tue, 30 Sep 2014 09:27:27 +0200
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Pankaj Dubey <pankaj.dubey@...sung.com>
Cc: 'Heiko Stübner' <heiko@...ech.de>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-samsung-soc@...r.kernel.org, arnd@...db.de,
b29396@...escale.com, lee.jones@...aro.org, tomasz.figa@...il.com,
linux@....linux.org.uk, vikas.sajjan@...sung.com,
naushad@...sung.com, thomas.ab@...sung.com, kgene.kim@...sung.com,
Li.Xiubo@...escale.com, geert+renesas@...der.be,
swarren@...dia.com, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v6] mfd: syscon: Decouple syscon interface from platform
devices
Hi Pankaj,
On Tue, 30 Sep 2014 09:33:38 +0530
Pankaj Dubey <pankaj.dubey@...sung.com> wrote:
> 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@...db.de>
> > > Suggested-by: Tomasz Figa <tomasz.figa@...il.com>
> > > Tested-by: Vivek Gautam <gautam.vivek@...sung.com>
> > > Tested-by: Javier Martinez Canillas <javier.martinez@...labora.co.uk>
> > > Signed-off-by: Pankaj Dubey <pankaj.dubey@...sung.com>
> > > ---
> >
> > On Rockchip boards during core clock init (aka before timers)
> > Tested-by: Heiko Stuebner <heiko@...ech.de>
> >
>
> Thanks for testing.
>
> > Except one issue described inline below
> > Reviewed-by: Heiko Stuebner <heiko@...ech.de>
> >
> >
> > And I'm really looking forward to having this in the kernel :-)
Me too :-).
> >
> > 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?
I too prefer the second solution, but it's not my call :-).
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists