[1/3] soc: fsl: add Platform PM driver QorIQ platforms

This driver is to provide a independent framework for PM service
provider and consumer to configure system level wake up feature. For
example, RCPM driver could register a callback function on this
platform first, and Flex timer driver who want to enable timer wake
up feature, will call generic API provided by this platform driver,
and then it will trigger RCPM driver to do it. The benefit is to
isolate the user and service, such as flex timer driver will not have
to know the implement details of wakeup function it require. Besides,
it is also easy for service side to upgrade its logic when design is
changed and remain user side unchanged.

Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
---
 drivers/soc/fsl/Kconfig   |   14 +++++
 drivers/soc/fsl/Makefile  |    1 +
 drivers/soc/fsl/plat_pm.c |  144 +++++++++++++++++++++++++++++++++++++++++++++
 include/soc/fsl/plat_pm.h |   22 +++++++
 4 files changed, 181 insertions(+), 0 deletions(-)
 create mode 100644 drivers/soc/fsl/plat_pm.c
 create mode 100644 include/soc/fsl/plat_pm.h

Please change your comments style.

On 2018/8/31 11:57, Ran Wang wrote:
> This driver is to provide a independent framework for PM service
> provider and consumer to configure system level wake up feature. For
> example, RCPM driver could register a callback function on this
> platform first, and Flex timer driver who want to enable timer wake
> up feature, will call generic API provided by this platform driver,
> and then it will trigger RCPM driver to do it. The benefit is to
> isolate the user and service, such as flex timer driver will not have
> to know the implement details of wakeup function it require. Besides,
> it is also easy for service side to upgrade its logic when design is
> changed and remain user side unchanged.
>
> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> ---
>  drivers/soc/fsl/Kconfig   |   14 +++++
>  drivers/soc/fsl/Makefile  |    1 +
>  drivers/soc/fsl/plat_pm.c |  144 +++++++++++++++++++++++++++++++++++++++++++++
>  include/soc/fsl/plat_pm.h |   22 +++++++
>  4 files changed, 181 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/soc/fsl/plat_pm.c
>  create mode 100644 include/soc/fsl/plat_pm.h
>
> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
> index 7a9fb9b..6517412 100644
> --- a/drivers/soc/fsl/Kconfig
> +++ b/drivers/soc/fsl/Kconfig
> @@ -16,3 +16,17 @@ config FSL_GUTS
>  	  Initially only reading SVR and registering soc device are supported.
>  	  Other guts accesses, such as reading RCW, should eventually be moved
>  	  into this driver as well.
> +
> +config FSL_PLAT_PM
> +	bool "Freescale platform PM framework"
> +	help
> +	  This driver is to provide a independent framework for PM service
> +	  provider and consumer to configure system level wake up feature. For
> +	  example, RCPM driver could register a callback function on this
> +	  platform first, and Flex timer driver who want to enable timer wake
> +	  up feature, will call generic API provided by this platform driver,
> +	  and then it will trigger RCPM driver to do it. The benefit is to
> +	  isolate the user and service, such as  flex timer driver will not
> +	  have to know the implement details of wakeup function it require.
> +	  Besides, it is also easy for service side to upgrade its logic when
> +	  design changed and remain user side unchanged.
> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> index 44b3beb..8f9db23 100644
> --- a/drivers/soc/fsl/Makefile
> +++ b/drivers/soc/fsl/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_FSL_DPAA)                 += qbman/
>  obj-$(CONFIG_QUICC_ENGINE)		+= qe/
>  obj-$(CONFIG_CPM)			+= qe/
>  obj-$(CONFIG_FSL_GUTS)			+= guts.o
> +obj-$(CONFIG_FSL_PLAT_PM)	+= plat_pm.o
> diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c
> new file mode 100644
> index 0000000..19ea14e
> --- /dev/null
> +++ b/drivers/soc/fsl/plat_pm.c
> @@ -0,0 +1,144 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// plat_pm.c - Freescale platform PM framework
> +//
> +// Copyright 2018 NXP
> +//
> +// Author: Ran Wang <ran.wang_1@nxp.com>,
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <soc/fsl/plat_pm.h>
> +
> +
> +struct plat_pm_t {
> +	struct list_head node;
> +	fsl_plat_pm_handle handle;
> +	void *handle_priv;
> +	spinlock_t	lock;
> +};
> +
> +static struct plat_pm_t plat_pm;
> +
> +// register_fsl_platform_wakeup_source - Register callback function to plat_pm
> +// @handle: Pointer to handle PM feature requirement
> +// @handle_priv: Handler specific data struct
> +//
> +// Return 0 on success other negative errno
> +int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> +		void *handle_priv)
> +{
> +	struct plat_pm_t *p;
> +	unsigned long	flags;
> +
> +	if (!handle) {
> +		pr_err("FSL plat_pm: Handler invalid, reject\n");
> +		return -EINVAL;
> +	}
> +
> +	p = kmalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	p->handle = handle;
> +	p->handle_priv = handle_priv;
> +
> +	spin_lock_irqsave(&plat_pm.lock, flags);
> +	list_add_tail(&p->node, &plat_pm.node);
> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);
> +
> +// Deregister_fsl_platform_wakeup_source - deregister callback function
> +// @handle_priv: Handler specific data struct
> +//
> +// Return 0 on success other negative errno
> +int deregister_fsl_platform_wakeup_source(void *handle_priv)
> +{
> +	struct plat_pm_t *p, *tmp;
> +	unsigned long	flags;
> +
> +	spin_lock_irqsave(&plat_pm.lock, flags);
> +	list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {
> +		if (p->handle_priv == handle_priv) {
> +			list_del(&p->node);
> +			kfree(p);
> +		}
> +	}
> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);
> +
> +// fsl_platform_wakeup_config - Configure wakeup source by calling handlers
> +// @dev: pointer to user's device struct
> +// @flag: to tell enable or disable wakeup source
> +//
> +// Return 0 on success other negative errno
> +int fsl_platform_wakeup_config(struct device *dev, bool flag)
> +{
> +	struct plat_pm_t *p;
> +	int ret;
> +	bool success_handled;
> +	unsigned long	flags;
> +
> +	success_handled = false;
> +
> +	// Will consider success if at least one callback return 0.
> +	// Also, rest handles still get oppertunity to be executed
> +	spin_lock_irqsave(&plat_pm.lock, flags);
> +	list_for_each_entry(p, &plat_pm.node, node) {
> +		if (p->handle) {
> +			ret = p->handle(dev, flag, p->handle_priv);
> +			if (!ret)
> +				success_handled = true;
Miss a break?

> +			else if (ret != -ENODEV) {
> +				pr_err("FSL plat_pm: Failed to config wakeup source:%d\n", ret);
Please unlock before return.

> +				return ret;
> +			}
> +		} else
> +			pr_warn("FSL plat_pm: Invalid handler detected, skip\n");
> +	}
> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> +
> +	if (success_handled == false) {
> +		pr_err("FSL plat_pm: Cannot find the matchhed handler for wakeup source config\n");
> +		return -ENODEV;
> +	}
Add this into the loop.
> +
> +	return 0;
> +}
> +
> +// fsl_platform_wakeup_enable - Enable wakeup source
> +// @dev: pointer to user's device struct
> +//
> +// Return 0 on success other negative errno
> +int fsl_platform_wakeup_enable(struct device *dev)
> +{
> +	return fsl_platform_wakeup_config(dev, true);
> +}
> +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);
> +
> +// fsl_platform_wakeup_disable - Disable wakeup source
> +// @dev: pointer to user's device struct
> +//
> +// Return 0 on success other negative errno
> +int fsl_platform_wakeup_disable(struct device *dev)
> +{
> +	return fsl_platform_wakeup_config(dev, false);
> +}
> +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);
> +
> +static int __init fsl_plat_pm_init(void)
> +{
> +	spin_lock_init(&plat_pm.lock);
> +	INIT_LIST_HEAD(&plat_pm.node);
> +	return 0;
> +}
> +
> +core_initcall(fsl_plat_pm_init);
> diff --git a/include/soc/fsl/plat_pm.h b/include/soc/fsl/plat_pm.h
> new file mode 100644
> index 0000000..bbe151e
> --- /dev/null
> +++ b/include/soc/fsl/plat_pm.h
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// plat_pm.h - Freescale platform PM Header
> +//
> +// Copyright 2018 NXP
> +//
> +// Author: Ran Wang <ran.wang_1@nxp.com>,
> +
> +#ifndef __FSL_PLAT_PM_H
> +#define __FSL_PLAT_PM_H
> +
> +typedef int (*fsl_plat_pm_handle)(struct device *dev, bool flag,
> +		void *handle_priv);
> +
> +int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> +		void *handle_priv);
> +int deregister_fsl_platform_wakeup_source(void *handle_priv);
> +int fsl_platform_wakeup_config(struct device *dev, bool flag);
> +int fsl_platform_wakeup_enable(struct device *dev);
> +int fsl_platform_wakeup_disable(struct device *dev);
> +
> +#endif	// __FSL_PLAT_PM_H

Hi Dongsheng

> On 2018/9/5 11:05, Dongsheng Wang wrote:
> 
> Please change your comments style.
> 
> On 2018/8/31 11:57, Ran Wang wrote:
> > This driver is to provide a independent framework for PM service
> > provider and consumer to configure system level wake up feature. For
> > example, RCPM driver could register a callback function on this
> > platform first, and Flex timer driver who want to enable timer wake up
> > feature, will call generic API provided by this platform driver, and
> > then it will trigger RCPM driver to do it. The benefit is to isolate
> > the user and service, such as flex timer driver will not have to know
> > the implement details of wakeup function it require. Besides, it is
> > also easy for service side to upgrade its logic when design is changed
> > and remain user side unchanged.
> >
> > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > ---
> >  drivers/soc/fsl/Kconfig   |   14 +++++
> >  drivers/soc/fsl/Makefile  |    1 +
> >  drivers/soc/fsl/plat_pm.c |  144
> +++++++++++++++++++++++++++++++++++++++++++++
> >  include/soc/fsl/plat_pm.h |   22 +++++++
> >  4 files changed, 181 insertions(+), 0 deletions(-)  create mode
> > 100644 drivers/soc/fsl/plat_pm.c  create mode 100644
> > include/soc/fsl/plat_pm.h
> >
> > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
> > 7a9fb9b..6517412 100644
> > --- a/drivers/soc/fsl/Kconfig
> > +++ b/drivers/soc/fsl/Kconfig
> > @@ -16,3 +16,17 @@ config FSL_GUTS
> >  	  Initially only reading SVR and registering soc device are supported.
> >  	  Other guts accesses, such as reading RCW, should eventually be
> moved
> >  	  into this driver as well.
> > +
> > +config FSL_PLAT_PM
> > +	bool "Freescale platform PM framework"
> > +	help
> > +	  This driver is to provide a independent framework for PM service
> > +	  provider and consumer to configure system level wake up feature.
> For
> > +	  example, RCPM driver could register a callback function on this
> > +	  platform first, and Flex timer driver who want to enable timer wake
> > +	  up feature, will call generic API provided by this platform driver,
> > +	  and then it will trigger RCPM driver to do it. The benefit is to
> > +	  isolate the user and service, such as  flex timer driver will not
> > +	  have to know the implement details of wakeup function it require.
> > +	  Besides, it is also easy for service side to upgrade its logic when
> > +	  design changed and remain user side unchanged.
> > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile index
> > 44b3beb..8f9db23 100644
> > --- a/drivers/soc/fsl/Makefile
> > +++ b/drivers/soc/fsl/Makefile
> > @@ -6,3 +6,4 @@ obj-$(CONFIG_FSL_DPAA)                 += qbman/
> >  obj-$(CONFIG_QUICC_ENGINE)		+= qe/
> >  obj-$(CONFIG_CPM)			+= qe/
> >  obj-$(CONFIG_FSL_GUTS)			+= guts.o
> > +obj-$(CONFIG_FSL_PLAT_PM)	+= plat_pm.o
> > diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c new
> > file mode 100644 index 0000000..19ea14e
> > --- /dev/null
> > +++ b/drivers/soc/fsl/plat_pm.c
> > @@ -0,0 +1,144 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// plat_pm.c - Freescale platform PM framework // // Copyright 2018
> > +NXP // // Author: Ran Wang <ran.wang_1@nxp.com>,
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <soc/fsl/plat_pm.h>
> > +
> > +
> > +struct plat_pm_t {
> > +	struct list_head node;
> > +	fsl_plat_pm_handle handle;
> > +	void *handle_priv;
> > +	spinlock_t	lock;
> > +};
> > +
> > +static struct plat_pm_t plat_pm;
> > +
> > +// register_fsl_platform_wakeup_source - Register callback function
> > +to plat_pm // @handle: Pointer to handle PM feature requirement //
> > +@handle_priv: Handler specific data struct // // Return 0 on success
> > +other negative errno int
> > +register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> > +		void *handle_priv)
> > +{
> > +	struct plat_pm_t *p;
> > +	unsigned long	flags;
> > +
> > +	if (!handle) {
> > +		pr_err("FSL plat_pm: Handler invalid, reject\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	p = kmalloc(sizeof(*p), GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> > +	p->handle = handle;
> > +	p->handle_priv = handle_priv;
> > +
> > +	spin_lock_irqsave(&plat_pm.lock, flags);
> > +	list_add_tail(&p->node, &plat_pm.node);
> > +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);
> > +
> > +// Deregister_fsl_platform_wakeup_source - deregister callback
> > +function // @handle_priv: Handler specific data struct // // Return 0
> > +on success other negative errno int
> > +deregister_fsl_platform_wakeup_source(void *handle_priv) {
> > +	struct plat_pm_t *p, *tmp;
> > +	unsigned long	flags;
> > +
> > +	spin_lock_irqsave(&plat_pm.lock, flags);
> > +	list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {
> > +		if (p->handle_priv == handle_priv) {
> > +			list_del(&p->node);
> > +			kfree(p);
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);
> > +
> > +// fsl_platform_wakeup_config - Configure wakeup source by calling
> > +handlers // @dev: pointer to user's device struct // @flag: to tell
> > +enable or disable wakeup source // // Return 0 on success other
> > +negative errno int fsl_platform_wakeup_config(struct device *dev,
> > +bool flag) {
> > +	struct plat_pm_t *p;
> > +	int ret;
> > +	bool success_handled;
> > +	unsigned long	flags;
> > +
> > +	success_handled = false;
> > +
> > +	// Will consider success if at least one callback return 0.
> > +	// Also, rest handles still get oppertunity to be executed
> > +	spin_lock_irqsave(&plat_pm.lock, flags);
> > +	list_for_each_entry(p, &plat_pm.node, node) {
> > +		if (p->handle) {
> > +			ret = p->handle(dev, flag, p->handle_priv);
> > +			if (!ret)
> > +				success_handled = true;
> Miss a break?

Actually my idea is to allow more than one registered handler to handle this
request, so I define a flag rather than return to indicated if there is at least one handler successfully
do it. This design might give more flexibility to framework when running.

> > +			else if (ret != -ENODEV) {
> > +				pr_err("FSL plat_pm: Failed to config wakeup
> source:%d\n", ret);
> Please unlock before return.

Yes, will fix it in next version, thanks for pointing out!

> > +				return ret;
> > +			}
> > +		} else
> > +			pr_warn("FSL plat_pm: Invalid handler detected,
> skip\n");
> > +	}
> > +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> > +
> > +	if (success_handled == false) {
> > +		pr_err("FSL plat_pm: Cannot find the matchhed handler for
> wakeup source config\n");
> > +		return -ENODEV;
> > +	}
> Add this into the loop.

My design is that if the 1st handler return -ENODEV to indicated this device it doesn't support, 
then the framework will continue try 2nd handler...

So I think it is needed to place this checking out of loop, what do you say?

Regards,
Ran
> > +
> > +	return 0;
> > +}
> > +
> > +// fsl_platform_wakeup_enable - Enable wakeup source // @dev: pointer
> > +to user's device struct // // Return 0 on success other negative
> > +errno int fsl_platform_wakeup_enable(struct device *dev) {
> > +	return fsl_platform_wakeup_config(dev, true); }
> > +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);
> > +
> > +// fsl_platform_wakeup_disable - Disable wakeup source // @dev:
> > +pointer to user's device struct // // Return 0 on success other
> > +negative errno int fsl_platform_wakeup_disable(struct device *dev) {
> > +	return fsl_platform_wakeup_config(dev, false); }
> > +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);
> > +
> > +static int __init fsl_plat_pm_init(void) {
> > +	spin_lock_init(&plat_pm.lock);
> > +	INIT_LIST_HEAD(&plat_pm.node);
> > +	return 0;
> > +}
> > +
> > +core_initcall(fsl_plat_pm_init);
> > diff --git a/include/soc/fsl/plat_pm.h b/include/soc/fsl/plat_pm.h new
> > file mode 100644 index 0000000..bbe151e
> > --- /dev/null
> > +++ b/include/soc/fsl/plat_pm.h
> > @@ -0,0 +1,22 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// plat_pm.h - Freescale platform PM Header // // Copyright 2018 NXP
> > +// // Author: Ran Wang <ran.wang_1@nxp.com>,
> > +
> > +#ifndef __FSL_PLAT_PM_H
> > +#define __FSL_PLAT_PM_H
> > +
> > +typedef int (*fsl_plat_pm_handle)(struct device *dev, bool flag,
> > +		void *handle_priv);
> > +
> > +int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> > +		void *handle_priv);
> > +int deregister_fsl_platform_wakeup_source(void *handle_priv); int
> > +fsl_platform_wakeup_config(struct device *dev, bool flag); int
> > +fsl_platform_wakeup_enable(struct device *dev); int
> > +fsl_platform_wakeup_disable(struct device *dev);
> > +
> > +#endif	// __FSL_PLAT_PM_H
>

On 2018/9/7 16:49, Ran Wang wrote:
> Hi Dongsheng
>
>> On 2018/9/5 11:05, Dongsheng Wang wrote:
>>
>> Please change your comments style.
>>
>> On 2018/8/31 11:57, Ran Wang wrote:
>>> This driver is to provide a independent framework for PM service
>>> provider and consumer to configure system level wake up feature. For
>>> example, RCPM driver could register a callback function on this
>>> platform first, and Flex timer driver who want to enable timer wake up
>>> feature, will call generic API provided by this platform driver, and
>>> then it will trigger RCPM driver to do it. The benefit is to isolate
>>> the user and service, such as flex timer driver will not have to know
>>> the implement details of wakeup function it require. Besides, it is
>>> also easy for service side to upgrade its logic when design is changed
>>> and remain user side unchanged.
>>>
>>> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
>>> ---
>>>  drivers/soc/fsl/Kconfig   |   14 +++++
>>>  drivers/soc/fsl/Makefile  |    1 +
>>>  drivers/soc/fsl/plat_pm.c |  144
>> +++++++++++++++++++++++++++++++++++++++++++++
>>>  include/soc/fsl/plat_pm.h |   22 +++++++
>>>  4 files changed, 181 insertions(+), 0 deletions(-)  create mode
>>> 100644 drivers/soc/fsl/plat_pm.c  create mode 100644
>>> include/soc/fsl/plat_pm.h
>>>
>>> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
>>> 7a9fb9b..6517412 100644
>>> --- a/drivers/soc/fsl/Kconfig
>>> +++ b/drivers/soc/fsl/Kconfig
>>> @@ -16,3 +16,17 @@ config FSL_GUTS
>>>  	  Initially only reading SVR and registering soc device are supported.
>>>  	  Other guts accesses, such as reading RCW, should eventually be
>> moved
>>>  	  into this driver as well.
>>> +
>>> +config FSL_PLAT_PM
>>> +	bool "Freescale platform PM framework"
>>> +	help
>>> +	  This driver is to provide a independent framework for PM service
>>> +	  provider and consumer to configure system level wake up feature.
>> For
>>> +	  example, RCPM driver could register a callback function on this
>>> +	  platform first, and Flex timer driver who want to enable timer wake
>>> +	  up feature, will call generic API provided by this platform driver,
>>> +	  and then it will trigger RCPM driver to do it. The benefit is to
>>> +	  isolate the user and service, such as  flex timer driver will not
>>> +	  have to know the implement details of wakeup function it require.
>>> +	  Besides, it is also easy for service side to upgrade its logic when
>>> +	  design changed and remain user side unchanged.
>>> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile index
>>> 44b3beb..8f9db23 100644
>>> --- a/drivers/soc/fsl/Makefile
>>> +++ b/drivers/soc/fsl/Makefile
>>> @@ -6,3 +6,4 @@ obj-$(CONFIG_FSL_DPAA)                 += qbman/
>>>  obj-$(CONFIG_QUICC_ENGINE)		+= qe/
>>>  obj-$(CONFIG_CPM)			+= qe/
>>>  obj-$(CONFIG_FSL_GUTS)			+= guts.o
>>> +obj-$(CONFIG_FSL_PLAT_PM)	+= plat_pm.o
>>> diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c new
>>> file mode 100644 index 0000000..19ea14e
>>> --- /dev/null
>>> +++ b/drivers/soc/fsl/plat_pm.c
>>> @@ -0,0 +1,144 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +//
>>> +// plat_pm.c - Freescale platform PM framework // // Copyright 2018
>>> +NXP // // Author: Ran Wang <ran.wang_1@nxp.com>,
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/device.h>
>>> +#include <linux/list.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/err.h>
>>> +#include <soc/fsl/plat_pm.h>
>>> +
>>> +
>>> +struct plat_pm_t {
>>> +	struct list_head node;
>>> +	fsl_plat_pm_handle handle;
>>> +	void *handle_priv;
>>> +	spinlock_t	lock;
>>> +};
>>> +
>>> +static struct plat_pm_t plat_pm;
>>> +
>>> +// register_fsl_platform_wakeup_source - Register callback function
>>> +to plat_pm // @handle: Pointer to handle PM feature requirement //
>>> +@handle_priv: Handler specific data struct // // Return 0 on success
>>> +other negative errno int
>>> +register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
>>> +		void *handle_priv)
>>> +{
>>> +	struct plat_pm_t *p;
>>> +	unsigned long	flags;
>>> +
>>> +	if (!handle) {
>>> +		pr_err("FSL plat_pm: Handler invalid, reject\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	p = kmalloc(sizeof(*p), GFP_KERNEL);
>>> +	if (!p)
>>> +		return -ENOMEM;
>>> +
>>> +	p->handle = handle;
>>> +	p->handle_priv = handle_priv;
>>> +
>>> +	spin_lock_irqsave(&plat_pm.lock, flags);
>>> +	list_add_tail(&p->node, &plat_pm.node);
>>> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);
>>> +
>>> +// Deregister_fsl_platform_wakeup_source - deregister callback
>>> +function // @handle_priv: Handler specific data struct // // Return 0
>>> +on success other negative errno int
>>> +deregister_fsl_platform_wakeup_source(void *handle_priv) {
>>> +	struct plat_pm_t *p, *tmp;
>>> +	unsigned long	flags;
>>> +
>>> +	spin_lock_irqsave(&plat_pm.lock, flags);
>>> +	list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {
>>> +		if (p->handle_priv == handle_priv) {
>>> +			list_del(&p->node);
>>> +			kfree(p);
>>> +		}
>>> +	}
>>> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);
>>> +
>>> +// fsl_platform_wakeup_config - Configure wakeup source by calling
>>> +handlers // @dev: pointer to user's device struct // @flag: to tell
>>> +enable or disable wakeup source // // Return 0 on success other
>>> +negative errno int fsl_platform_wakeup_config(struct device *dev,
>>> +bool flag) {
>>> +	struct plat_pm_t *p;
>>> +	int ret;
>>> +	bool success_handled;
>>> +	unsigned long	flags;
>>> +
>>> +	success_handled = false;
>>> +
>>> +	// Will consider success if at least one callback return 0.
>>> +	// Also, rest handles still get oppertunity to be executed
>>> +	spin_lock_irqsave(&plat_pm.lock, flags);
>>> +	list_for_each_entry(p, &plat_pm.node, node) {
>>> +		if (p->handle) {
>>> +			ret = p->handle(dev, flag, p->handle_priv);
>>> +			if (!ret)
>>> +				success_handled = true;
>> Miss a break?
> Actually my idea is to allow more than one registered handler to handle this
> request, so I define a flag rather than return to indicated if there is at least one handler successfully
> do it. This design might give more flexibility to framework when running.
There is only one flag(success_handled) here, how did know which handler
failed?

BTW, I don't think we need this flag. We can only use the return value.

Cheers,
Dongsheng

>>> +			else if (ret != -ENODEV) {
>>> +				pr_err("FSL plat_pm: Failed to config wakeup
>> source:%d\n", ret);
>> Please unlock before return.
> Yes, will fix it in next version, thanks for pointing out!
>
>>> +				return ret;
>>> +			}
>>> +		} else
>>> +			pr_warn("FSL plat_pm: Invalid handler detected,
>> skip\n");
>>> +	}
>>> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
>>> +
>>> +	if (success_handled == false) {
>>> +		pr_err("FSL plat_pm: Cannot find the matchhed handler for
>> wakeup source config\n");
>>> +		return -ENODEV;
>>> +	}
>> Add this into the loop.
> My design is that if the 1st handler return -ENODEV to indicated this device it doesn't support, 
> then the framework will continue try 2nd handler...
>
> So I think it is needed to place this checking out of loop, what do you say?
>
> Regards,
> Ran
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +// fsl_platform_wakeup_enable - Enable wakeup source // @dev: pointer
>>> +to user's device struct // // Return 0 on success other negative
>>> +errno int fsl_platform_wakeup_enable(struct device *dev) {
>>> +	return fsl_platform_wakeup_config(dev, true); }
>>> +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);
>>> +
>>> +// fsl_platform_wakeup_disable - Disable wakeup source // @dev:
>>> +pointer to user's device struct // // Return 0 on success other
>>> +negative errno int fsl_platform_wakeup_disable(struct device *dev) {
>>> +	return fsl_platform_wakeup_config(dev, false); }
>>> +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);
>>> +
>>> +static int __init fsl_plat_pm_init(void) {
>>> +	spin_lock_init(&plat_pm.lock);
>>> +	INIT_LIST_HEAD(&plat_pm.node);
>>> +	return 0;
>>> +}
>>> +
>>> +core_initcall(fsl_plat_pm_init);
>>> diff --git a/include/soc/fsl/plat_pm.h b/include/soc/fsl/plat_pm.h new
>>> file mode 100644 index 0000000..bbe151e
>>> --- /dev/null
>>> +++ b/include/soc/fsl/plat_pm.h
>>> @@ -0,0 +1,22 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +//
>>> +// plat_pm.h - Freescale platform PM Header // // Copyright 2018 NXP
>>> +// // Author: Ran Wang <ran.wang_1@nxp.com>,
>>> +
>>> +#ifndef __FSL_PLAT_PM_H
>>> +#define __FSL_PLAT_PM_H
>>> +
>>> +typedef int (*fsl_plat_pm_handle)(struct device *dev, bool flag,
>>> +		void *handle_priv);
>>> +
>>> +int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
>>> +		void *handle_priv);
>>> +int deregister_fsl_platform_wakeup_source(void *handle_priv); int
>>> +fsl_platform_wakeup_config(struct device *dev, bool flag); int
>>> +fsl_platform_wakeup_enable(struct device *dev); int
>>> +fsl_platform_wakeup_disable(struct device *dev);
>>> +
>>> +#endif	// __FSL_PLAT_PM_H
>

On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
> This driver is to provide a independent framework for PM service
> provider and consumer to configure system level wake up feature. For
> example, RCPM driver could register a callback function on this
> platform first, and Flex timer driver who want to enable timer wake
> up feature, will call generic API provided by this platform driver,
> and then it will trigger RCPM driver to do it. The benefit is to
> isolate the user and service, such as flex timer driver will not have
> to know the implement details of wakeup function it require. Besides,
> it is also easy for service side to upgrade its logic when design is
> changed and remain user side unchanged.
> 
> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> ---
>  drivers/soc/fsl/Kconfig   |   14 +++++
>  drivers/soc/fsl/Makefile  |    1 +
>  drivers/soc/fsl/plat_pm.c |  144
> +++++++++++++++++++++++++++++++++++++++++++++
>  include/soc/fsl/plat_pm.h |   22 +++++++
>  4 files changed, 181 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/soc/fsl/plat_pm.c
>  create mode 100644 include/soc/fsl/plat_pm.h
> 
> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
> index 7a9fb9b..6517412 100644
> --- a/drivers/soc/fsl/Kconfig
> +++ b/drivers/soc/fsl/Kconfig
> @@ -16,3 +16,17 @@ config FSL_GUTS
>  	  Initially only reading SVR and registering soc device are
> supported.
>  	  Other guts accesses, such as reading RCW, should eventually be
> moved
>  	  into this driver as well.
+
> +config FSL_PLAT_PM
> +	bool "Freescale platform PM framework"

This name seems to be simultaneously too generic (for something that is likely
intended only for use with certain Freescale/NXP chip families) and too
specific (for something that seems to be general infrastructure with no real
hardware dependencies).

What specific problems with Linux's generic wakeup infrastructure is this
trying to solve, and why would those problems not be better solved there?

Also, you should CC linux-pm on these patches.

-Scott

Hi Dongsheng,

On 2018/9/7 18:16, Dongsheng Wang wrote:
> 
> On 2018/9/7 16:49, Ran Wang wrote:
> > Hi Dongsheng
> >
> >> On 2018/9/5 11:05, Dongsheng Wang wrote:
> >>
> >> Please change your comments style.
> >>
> >> On 2018/8/31 11:57, Ran Wang wrote:
> >>> This driver is to provide a independent framework for PM service
> >>> provider and consumer to configure system level wake up feature. For
> >>> example, RCPM driver could register a callback function on this
> >>> platform first, and Flex timer driver who want to enable timer wake
> >>> up feature, will call generic API provided by this platform driver,
> >>> and then it will trigger RCPM driver to do it. The benefit is to
> >>> isolate the user and service, such as flex timer driver will not
> >>> have to know the implement details of wakeup function it require.
> >>> Besides, it is also easy for service side to upgrade its logic when
> >>> design is changed and remain user side unchanged.
> >>>
> >>> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> >>> ---
> >>>  drivers/soc/fsl/Kconfig   |   14 +++++
> >>>  drivers/soc/fsl/Makefile  |    1 +
> >>>  drivers/soc/fsl/plat_pm.c |  144
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>>  include/soc/fsl/plat_pm.h |   22 +++++++
> >>>  4 files changed, 181 insertions(+), 0 deletions(-)  create mode
> >>> 100644 drivers/soc/fsl/plat_pm.c  create mode 100644
> >>> include/soc/fsl/plat_pm.h
> >>>
> >>> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
> >>> 7a9fb9b..6517412 100644
> >>> --- a/drivers/soc/fsl/Kconfig
> >>> +++ b/drivers/soc/fsl/Kconfig
> >>> @@ -16,3 +16,17 @@ config FSL_GUTS
> >>>  	  Initially only reading SVR and registering soc device are supported.
> >>>  	  Other guts accesses, such as reading RCW, should eventually be
> >> moved
> >>>  	  into this driver as well.
> >>> +
> >>> +config FSL_PLAT_PM
> >>> +	bool "Freescale platform PM framework"
> >>> +	help
> >>> +	  This driver is to provide a independent framework for PM service
> >>> +	  provider and consumer to configure system level wake up feature.
> >> For
> >>> +	  example, RCPM driver could register a callback function on this
> >>> +	  platform first, and Flex timer driver who want to enable timer wake
> >>> +	  up feature, will call generic API provided by this platform driver,
> >>> +	  and then it will trigger RCPM driver to do it. The benefit is to
> >>> +	  isolate the user and service, such as  flex timer driver will not
> >>> +	  have to know the implement details of wakeup function it require.
> >>> +	  Besides, it is also easy for service side to upgrade its logic when
> >>> +	  design changed and remain user side unchanged.
> >>> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> >>> index
> >>> 44b3beb..8f9db23 100644
> >>> --- a/drivers/soc/fsl/Makefile
> >>> +++ b/drivers/soc/fsl/Makefile
> >>> @@ -6,3 +6,4 @@ obj-$(CONFIG_FSL_DPAA)                 += qbman/
> >>>  obj-$(CONFIG_QUICC_ENGINE)		+= qe/
> >>>  obj-$(CONFIG_CPM)			+= qe/
> >>>  obj-$(CONFIG_FSL_GUTS)			+= guts.o
> >>> +obj-$(CONFIG_FSL_PLAT_PM)	+= plat_pm.o
> >>> diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c
> >>> new file mode 100644 index 0000000..19ea14e
> >>> --- /dev/null
> >>> +++ b/drivers/soc/fsl/plat_pm.c
> >>> @@ -0,0 +1,144 @@
> >>> +// SPDX-License-Identifier: GPL-2.0 // // plat_pm.c - Freescale
> >>> +platform PM framework // // Copyright 2018 NXP // // Author: Ran
> >>> +Wang <ran.wang_1@nxp.com>,
> >>> +
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/device.h>
> >>> +#include <linux/list.h>
> >>> +#include <linux/slab.h>
> >>> +#include <linux/err.h>
> >>> +#include <soc/fsl/plat_pm.h>
> >>> +
> >>> +
> >>> +struct plat_pm_t {
> >>> +	struct list_head node;
> >>> +	fsl_plat_pm_handle handle;
> >>> +	void *handle_priv;
> >>> +	spinlock_t	lock;
> >>> +};
> >>> +
> >>> +static struct plat_pm_t plat_pm;
> >>> +
> >>> +// register_fsl_platform_wakeup_source - Register callback function
> >>> +to plat_pm // @handle: Pointer to handle PM feature requirement //
> >>> +@handle_priv: Handler specific data struct // // Return 0 on
> >>> +success other negative errno int
> >>> +register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> >>> +		void *handle_priv)
> >>> +{
> >>> +	struct plat_pm_t *p;
> >>> +	unsigned long	flags;
> >>> +
> >>> +	if (!handle) {
> >>> +		pr_err("FSL plat_pm: Handler invalid, reject\n");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	p = kmalloc(sizeof(*p), GFP_KERNEL);
> >>> +	if (!p)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	p->handle = handle;
> >>> +	p->handle_priv = handle_priv;
> >>> +
> >>> +	spin_lock_irqsave(&plat_pm.lock, flags);
> >>> +	list_add_tail(&p->node, &plat_pm.node);
> >>> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);
> >>> +
> >>> +// Deregister_fsl_platform_wakeup_source - deregister callback
> >>> +function // @handle_priv: Handler specific data struct // // Return
> >>> +0 on success other negative errno int
> >>> +deregister_fsl_platform_wakeup_source(void *handle_priv) {
> >>> +	struct plat_pm_t *p, *tmp;
> >>> +	unsigned long	flags;
> >>> +
> >>> +	spin_lock_irqsave(&plat_pm.lock, flags);
> >>> +	list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {
> >>> +		if (p->handle_priv == handle_priv) {
> >>> +			list_del(&p->node);
> >>> +			kfree(p);
> >>> +		}
> >>> +	}
> >>> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);
> >>> +
> >>> +// fsl_platform_wakeup_config - Configure wakeup source by calling
> >>> +handlers // @dev: pointer to user's device struct // @flag: to tell
> >>> +enable or disable wakeup source // // Return 0 on success other
> >>> +negative errno int fsl_platform_wakeup_config(struct device *dev,
> >>> +bool flag) {
> >>> +	struct plat_pm_t *p;
> >>> +	int ret;
> >>> +	bool success_handled;
> >>> +	unsigned long	flags;
> >>> +
> >>> +	success_handled = false;
> >>> +
> >>> +	// Will consider success if at least one callback return 0.
> >>> +	// Also, rest handles still get oppertunity to be executed
> >>> +	spin_lock_irqsave(&plat_pm.lock, flags);
> >>> +	list_for_each_entry(p, &plat_pm.node, node) {
> >>> +		if (p->handle) {
> >>> +			ret = p->handle(dev, flag, p->handle_priv);
> >>> +			if (!ret)
> >>> +				success_handled = true;
> >> Miss a break?
> > Actually my idea is to allow more than one registered handler to
> > handle this request, so I define a flag rather than return to
> > indicated if there is at least one handler successfully do it. This design
> might give more flexibility to framework when running.
> There is only one flag(success_handled) here, how did know which handler
> failed?
> 
> BTW, I don't think we need this flag. We can only use the return value.

Well, the plat_pm driver will not handle most errors returned by registered
handlers, except -NODEV. For -NODEV, plat_pm driver consider that handler
cannot support this request and will go to call next one. 

Besides, actually it doesn't restrict that request can be served by only one 
handler. So I add that flag to cover the case of more than one handler can 
successfully support and others might return -NODEV.

Regards,
Ran

> Cheers,
> Dongsheng
> 
> >>> +			else if (ret != -ENODEV) {
> >>> +				pr_err("FSL plat_pm: Failed to config wakeup
> >> source:%d\n", ret);
> >> Please unlock before return.
> > Yes, will fix it in next version, thanks for pointing out!
> >
> >>> +				return ret;
> >>> +			}
> >>> +		} else
> >>> +			pr_warn("FSL plat_pm: Invalid handler detected,
> >> skip\n");
> >>> +	}
> >>> +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> >>> +
> >>> +	if (success_handled == false) {
> >>> +		pr_err("FSL plat_pm: Cannot find the matchhed handler for
> >> wakeup source config\n");
> >>> +		return -ENODEV;
> >>> +	}
> >> Add this into the loop.
> > My design is that if the 1st handler return -ENODEV to indicated this
> > device it doesn't support, then the framework will continue try 2nd
> handler...
> >
> > So I think it is needed to place this checking out of loop, what do you say?
> >
> > Regards,
> > Ran
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +// fsl_platform_wakeup_enable - Enable wakeup source // @dev:
> >>> +pointer to user's device struct // // Return 0 on success other
> >>> +negative errno int fsl_platform_wakeup_enable(struct device *dev) {
> >>> +	return fsl_platform_wakeup_config(dev, true); }
> >>> +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);
> >>> +
> >>> +// fsl_platform_wakeup_disable - Disable wakeup source // @dev:
> >>> +pointer to user's device struct // // Return 0 on success other
> >>> +negative errno int fsl_platform_wakeup_disable(struct device *dev) {
> >>> +	return fsl_platform_wakeup_config(dev, false); }
> >>> +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);
> >>> +
> >>> +static int __init fsl_plat_pm_init(void) {
> >>> +	spin_lock_init(&plat_pm.lock);
> >>> +	INIT_LIST_HEAD(&plat_pm.node);
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +core_initcall(fsl_plat_pm_init);
> >>> diff --git a/include/soc/fsl/plat_pm.h b/include/soc/fsl/plat_pm.h
> >>> new file mode 100644 index 0000000..bbe151e
> >>> --- /dev/null
> >>> +++ b/include/soc/fsl/plat_pm.h
> >>> @@ -0,0 +1,22 @@
> >>> +// SPDX-License-Identifier: GPL-2.0 // // plat_pm.h - Freescale
> >>> +platform PM Header // // Copyright 2018 NXP // // Author: Ran Wang
> >>> +<ran.wang_1@nxp.com>,
> >>> +
> >>> +#ifndef __FSL_PLAT_PM_H
> >>> +#define __FSL_PLAT_PM_H
> >>> +
> >>> +typedef int (*fsl_plat_pm_handle)(struct device *dev, bool flag,
> >>> +		void *handle_priv);
> >>> +
> >>> +int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> >>> +		void *handle_priv);
> >>> +int deregister_fsl_platform_wakeup_source(void *handle_priv); int
> >>> +fsl_platform_wakeup_config(struct device *dev, bool flag); int
> >>> +fsl_platform_wakeup_enable(struct device *dev); int
> >>> +fsl_platform_wakeup_disable(struct device *dev);
> >>> +
> >>> +#endif	// __FSL_PLAT_PM_H
> >

Hi Scott,

On 2018/9/8 4:35, Scott Wood wrote:
> 
> On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
> > This driver is to provide a independent framework for PM service
> > provider and consumer to configure system level wake up feature. For
> > example, RCPM driver could register a callback function on this
> > platform first, and Flex timer driver who want to enable timer wake up
> > feature, will call generic API provided by this platform driver, and
> > then it will trigger RCPM driver to do it. The benefit is to isolate
> > the user and service, such as flex timer driver will not have to know
> > the implement details of wakeup function it require. Besides, it is
> > also easy for service side to upgrade its logic when design is changed
> > and remain user side unchanged.
> >
> > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > ---
> >  drivers/soc/fsl/Kconfig   |   14 +++++
> >  drivers/soc/fsl/Makefile  |    1 +
> >  drivers/soc/fsl/plat_pm.c |  144
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  include/soc/fsl/plat_pm.h |   22 +++++++
> >  4 files changed, 181 insertions(+), 0 deletions(-)  create mode
> > 100644 drivers/soc/fsl/plat_pm.c  create mode 100644
> > include/soc/fsl/plat_pm.h
> >
> > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
> > 7a9fb9b..6517412 100644
> > --- a/drivers/soc/fsl/Kconfig
> > +++ b/drivers/soc/fsl/Kconfig
> > @@ -16,3 +16,17 @@ config FSL_GUTS
> >  	  Initially only reading SVR and registering soc device are
> > supported.
> >  	  Other guts accesses, such as reading RCW, should eventually be
> > moved
> >  	  into this driver as well.
> +
> > +config FSL_PLAT_PM
> > +	bool "Freescale platform PM framework"
> 
> This name seems to be simultaneously too generic (for something that is
> likely intended only for use with certain Freescale/NXP chip families) and too
> specific (for something that seems to be general infrastructure with no real
> hardware dependencies).

Yes, this driver has no real HW dependencies at all. But we'd like to introduce it
to help providing more flexibility & generic on FSL PM feature configure (so far 
we have RCPM on system wakeup source control). I think it's good
for driver/IP porting among different SoC in the future. As to the name, do you
have better suggestion?

> What specific problems with Linux's generic wakeup infrastructure is this
> trying to solve, and why would those problems not be better solved there?

Actually, I am not sure if generic wakeup infrastructure have this kind of PM feature
(keep specific IP alive during system suspend, could you please show me?).
And I think it is not common requirement, so I decide to put it in FSL folder. 

> Also, you should CC linux-pm on these patches.

Yes, thanks for suggestion

Regards,
Ran

> -Scott