Re: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices
Hi Pankaj,
Am Dienstag, 30. September 2014, 09:33:38 schrieb Pankaj Dubey:
> 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
> > > Suggested-by: Tomasz Figa
> > > Tested-by: Vivek Gautam
> > > Tested-by: Javier Martinez Canillas
> > > Signed-off-by: Pankaj Dubey
> > > ---
> >
> > On Rockchip boards during core clock init (aka before timers)
> > Tested-by: Heiko Stuebner
>
> Thanks for testing.
>
> > Except one issue described inline below
> > Reviewed-by: Heiko Stuebner
> >
> >
> > 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
> > > #include
> > > #include
> > >
> > > +#include
> > >
> > > #include
> > > #include
> > > #include
> > >
> > > @@ -22,31 +23,104 @@
> > >
> > > #include
> > > #include
> > > #include
> > >
> > > +#include
> > >
> > > 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,
Re: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices
Hi Pankaj,
On Tue, 30 Sep 2014 09:33:38 +0530
Pankaj Dubey 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
> > > Suggested-by: Tomasz Figa
> > > Tested-by: Vivek Gautam
> > > Tested-by: Javier Martinez Canillas
> > > Signed-off-by: Pankaj Dubey
> > > ---
> >
> > On Rockchip boards during core clock init (aka before timers)
> > Tested-by: Heiko Stuebner
> >
>
> Thanks for testing.
>
> > Except one issue described inline below
> > Reviewed-by: Heiko Stuebner
> >
> >
> > 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
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -22,31 +23,104 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > 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
RE: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices
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
> > Suggested-by: Tomasz Figa
> > Tested-by: Vivek Gautam
> > Tested-by: Javier Martinez Canillas
> > Signed-off-by: Pankaj Dubey
> > ---
>
> On Rockchip boards during core clock init (aka before timers)
> Tested-by: Heiko Stuebner
>
Thanks for testing.
> Except one issue described inline below
> Reviewed-by: Heiko Stuebner
>
>
> 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -22,31 +23,104 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > 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
Re: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices
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
> Suggested-by: Tomasz Figa
> Tested-by: Vivek Gautam
> Tested-by: Javier Martinez Canillas
> Signed-off-by: Pankaj Dubey
> ---
On Rockchip boards during core clock init (aka before timers)
Tested-by: Heiko Stuebner
Except one issue described inline below
Reviewed-by: Heiko Stuebner
And I'm really looking forward to having this in the kernel :-)
Thanks for working on this
Heiko
> Patch v5 and related discussions can be found here [1].
>
> Change since v5:
> - Dropping creation of dummy platform device in of_syscon_register.
> - As we are changing syscon to decouple from platform_device, creation of
>dummy platform_device does not look good option, and as suggested by
> Arnd, I made another attempt so that regmap_mmio_init API should work with
> NULL dev pointer itself. Since regmap needs to know about Syscon device
> node properties so let's parse device node of syscon in syscon itself for
> any such properties and using regmap_config parameter pass all such
> information to regmap. Other concern of crashes due to NULL dev pointer in
> regmap already addressed in separate patches of regmap. Please see [2] and
> [3].
>
>
> 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/22/12
> [2]: https://lkml.org/lkml/2014/9/18/130
> [3]: https://lkml.org/lkml/2014/9/27/2
>
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -22,31 +23,104 @@
> #include
> #include
> #include
> +#include
>
> 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 sy
Re: [PATCH v6] mfd: syscon: Decouple syscon interface from platform devices
On Monday 29 September 2014 14:17:38 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 completely remove platform driver of syscon, and keep only helper > functions to get regmap handles. > > Suggested-by: Arnd Bergmann > Suggested-by: Tomasz Figa > Tested-by: Vivek Gautam > Tested-by: Javier Martinez Canillas > Signed-off-by: Pankaj Dubey > --- > Patch v5 and related discussions can be found here [1]. > > Change since v5: > - Dropping creation of dummy platform device in of_syscon_register. > - As we are changing syscon to decouple from platform_device, creation of >dummy platform_device does not look good option, and as suggested by Arnd, >I made another attempt so that regmap_mmio_init API should work with NULL >dev pointer itself. Since regmap needs to know about Syscon device node >properties so let's parse device node of syscon in syscon itself for any >such properties and using regmap_config parameter pass all such information >to regmap. Other concern of crashes due to NULL dev pointer in regmap > already >addressed in separate patches of regmap. Please see [2] and [3]. > > > Reviewed-by: Arnd Bergmann Looks good to me, thanks a lot! Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
