[PATCH 1/3] param: add dev_add_param_tristate(_ro) helpers

[PATCH 1/3] param: add dev_add_param_tristate(_ro) helpers

[Ahmad Fatoum]

This is can be considered an extension to the dev_add_param_bool
interfaces with a third value that's "unknown".
This can be used for cases, where barebox can flip a bit somewhere,
but it has no way of knowing what the initial state of the bit was,
e.g. turn on/off the watchdog, but no watchdog status.
Turn on/off a co-processor, but no co-processor online status.
And so on. Not providing a way to customize the "unknown" string
is a deliberate choice, so future device parameters follow the same
naming scheme.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 include/param.h | 24 ++++++++++++++++++++++++
 lib/parameter.c | 22 ++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/param.h b/include/param.h
index 4ac502e726c0..d75f50ea3e60 100644
--- a/include/param.h
+++ b/include/param.h
@@ -63,6 +63,16 @@ struct param_d *dev_add_param_enum(struct device_d *dev, const char *name,
 		int (*get)(struct param_d *p, void *priv),
 		int *value, const char * const *names, int max, void *priv);
 
+enum param_tristate { PARAM_TRISTATE_UNKNOWN, PARAM_TRISTATE_TRUE, PARAM_TRISTATE_FALSE };
+
+struct param_d *dev_add_param_tristate(struct device_d *dev, const char *name,
+		int (*set)(struct param_d *p, void *priv),
+		int (*get)(struct param_d *p, void *priv),
+		int *value, void *priv);
+
+struct param_d *dev_add_param_tristate_ro(struct device_d *dev, const char *name,
+		int *value);
+
 struct param_d *dev_add_param_bitmask(struct device_d *dev, const char *name,
 		int (*set)(struct param_d *p, void *priv),
 		int (*get)(struct param_d *p, void *priv),
@@ -144,6 +154,20 @@ static inline struct param_d *dev_add_param_bitmask(struct device_d *dev, const
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct param_d *dev_add_param_tristate(struct device_d *dev, const char *name,
+		int (*set)(struct param_d *p, void *priv),
+		int (*get)(struct param_d *p, void *priv),
+		int *value, void *priv)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline struct param_d *dev_add_param_tristate_ro(struct device_d *dev, const char *name,
+		int *value)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline struct param_d *dev_add_param_ip(struct device_d *dev, const char *name,
 		int (*set)(struct param_d *p, void *priv),
 		int (*get)(struct param_d *p, void *priv),
diff --git a/lib/parameter.c b/lib/parameter.c
index fdbb2e71d15d..22695634e5f1 100644
--- a/lib/parameter.c
+++ b/lib/parameter.c
@@ -588,6 +588,28 @@ struct param_d *dev_add_param_enum(struct device_d *dev, const char *name,
 	return &pe->param;
 }
 
+static const char *const tristate_names[] = {
+	[PARAM_TRISTATE_UNKNOWN] = "unknown",
+	[PARAM_TRISTATE_TRUE] = "1",
+	[PARAM_TRISTATE_FALSE] = "0",
+};
+
+struct param_d *dev_add_param_tristate(struct device_d *dev, const char *name,
+		int (*set)(struct param_d *p, void *priv),
+		int (*get)(struct param_d *p, void *priv),
+		int *value, void *priv)
+{
+	return dev_add_param_enum(dev, name, set, get, value, tristate_names,
+				  ARRAY_SIZE(tristate_names), priv);
+}
+
+struct param_d *dev_add_param_tristate_ro(struct device_d *dev, const char *name,
+		int *value)
+{
+	return dev_add_param_enum_ro(dev, name, value, tristate_names,
+				     ARRAY_SIZE(tristate_names));
+}
+
 struct param_bitmask {
 	struct param_d param;
 	unsigned long *value;
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] watchdog: core: use new dev_add_param_tristate helper for .running param

[Ahmad Fatoum]

Previous commit added a dev_add_param_tristate_ro that can be readily
used instead of the enum parameter here. Use it.
This also fixes the issue that running_names had external linkage.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 drivers/watchdog/wd_core.c | 9 +--------
 include/watchdog.h         | 5 ++++-
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index fcead1175558..b6e2a37b1f0c 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -152,12 +152,6 @@ static unsigned int dev_get_watchdog_priority(struct device_d *dev)
 	return priority;
 }
 
-const char *running_names[] = {
-	[WDOG_HW_RUNNING_UNSUPPORTED] = "unknown",
-	[WDOG_HW_RUNNING] = "1",
-	[WDOG_HW_NOT_RUNNING] = "0",
-};
-
 int watchdog_register(struct watchdog *wd)
 {
 	struct param_d *p;
@@ -176,8 +170,7 @@ int watchdog_register(struct watchdog *wd)
 	if (ret)
 		return ret;
 
-	p = dev_add_param_enum_ro(&wd->dev, "running", &wd->running,
-				  running_names, ARRAY_SIZE(running_names));
+	p = dev_add_param_tristate_ro(&wd->dev, "running", &wd->running);
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
diff --git a/include/watchdog.h b/include/watchdog.h
index 5790205a487b..9741570ce229 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -15,9 +15,12 @@
 
 #include <poller.h>
 #include <driver.h>
+#include <param.h>
 
 enum wdog_hw_runnning {
-	 WDOG_HW_RUNNING_UNSUPPORTED, WDOG_HW_RUNNING, WDOG_HW_NOT_RUNNING
+	 WDOG_HW_RUNNING_UNSUPPORTED = PARAM_TRISTATE_UNKNOWN,
+	 WDOG_HW_RUNNING = PARAM_TRISTATE_TRUE,
+	 WDOG_HW_NOT_RUNNING = PARAM_TRISTATE_FALSE
 };
 
 struct watchdog {
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] remoteproc: add .stop device parameter for stopping remote processor

[Ahmad Fatoum]

Both the STM32 and i.MX7 remote proc drivers populate the .stop member
in the struct rproc, but it's not used anywhere. The firmware API is not
really fitting to 'unload' firmware. Add instead a device parameter to
stop a remote processor, e.g. remoteproc0.stop=1. This is similar to the
probe command used with MMCs.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 drivers/remoteproc/remoteproc_core.c | 30 +++++++++++++++++++++++-----
 include/linux/remoteproc.h           |  2 ++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8a28c1bafc1b..e031640bc7a0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -101,6 +101,8 @@ static int rproc_firmware_finish(struct firmware_handler *fh)
 	fw.size = rproc->fw_buf_ofs;
 
 	ret = rproc_start(rproc, &fw);
+	if (ret == 0)
+		rproc->stop = PARAM_TRISTATE_FALSE;
 
 	kfree(rproc->fw_buf);
 
@@ -120,6 +122,19 @@ static int rproc_register_dev(struct rproc *rproc, const char *alias)
 	return register_device(&rproc->dev);
 }
 
+static int rproc_set_stop(struct param_d *param, void *priv)
+{
+	struct rproc *rproc = priv;
+	int (*stop)(struct rproc *);
+
+	stop = rproc->ops->stop;
+
+	if (!stop)
+		return -ENOSYS;
+
+	return stop(rproc);
+}
+
 int rproc_add(struct rproc *rproc)
 {
 	struct device_d *dev = &rproc->dev;
@@ -142,12 +157,17 @@ int rproc_add(struct rproc *rproc)
 	fh->close = rproc_firmware_finish;
 
 	ret = firmwaremgr_register(fh);
-	if (ret)
-		dev_err(dev, "filed to register firmware handler %s\n", rproc->name);
-	else
-		dev_info(dev, "%s is available\n", rproc->name);
+	if (ret) {
+		dev_err(dev, "failed to register firmware handler %s\n", rproc->name);
+		return ret;
+	}
 
-	return ret;
+	rproc->stop = PARAM_TRISTATE_UNKNOWN;
+	dev_add_param_tristate(&rproc->dev, "stop", rproc_set_stop, NULL,
+			   &rproc->stop, rproc);
+
+	dev_info(dev, "%s is available\n", rproc->name);
+	return 0;
 }
 
 struct rproc *rproc_alloc(struct device_d *dev, const char *name,
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index c6264d1c0a49..af35837fb39a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,8 @@ struct rproc {
 
 	void *fw_buf;
 	size_t fw_buf_ofs;
+
+	int stop;
 };
 
 struct rproc *rproc_alloc(struct device_d *dev, const char *name,
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] remoteproc: add .stop device parameter for stopping remote processor

[Sascha Hauer]

On Thu, Nov 21, 2019 at 09:40:05AM +0100, Ahmad Fatoum wrote:
> Both the STM32 and i.MX7 remote proc drivers populate the .stop member
> in the struct rproc, but it's not used anywhere.

The .stop member in struct rproc is introduced in this patch.

> The firmware API is not
> really fitting to 'unload' firmware. Add instead a device parameter to
> stop a remote processor, e.g. remoteproc0.stop=1. This is similar to the
> probe command used with MMCs.
> 
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
>  drivers/remoteproc/remoteproc_core.c | 30 +++++++++++++++++++++++-----
>  include/linux/remoteproc.h           |  2 ++
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 8a28c1bafc1b..e031640bc7a0 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -101,6 +101,8 @@ static int rproc_firmware_finish(struct firmware_handler *fh)
>  	fw.size = rproc->fw_buf_ofs;
>  
>  	ret = rproc_start(rproc, &fw);
> +	if (ret == 0)
> +		rproc->stop = PARAM_TRISTATE_FALSE;

Can we use positive logic here? "Status Stopped is false" is harder to
read than just "running" or "started".

  
>  	kfree(rproc->fw_buf);
>  
> @@ -120,6 +122,19 @@ static int rproc_register_dev(struct rproc *rproc, const char *alias)
>  	return register_device(&rproc->dev);
>  }
>  
> +static int rproc_set_stop(struct param_d *param, void *priv)
> +{
> +	struct rproc *rproc = priv;
> +	int (*stop)(struct rproc *);
> +
> +	stop = rproc->ops->stop;
> +
> +	if (!stop)
> +		return -ENOSYS;
> +
> +	return stop(rproc);
> +}

I would assume that when I can stop the remote processor with this
parameter I should be able to start it here as well, no?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] remoteproc: add .stop device parameter for stopping remote processor

[Ahmad Fatoum]

On 11/25/19 9:28 AM, Sascha Hauer wrote:
> On Thu, Nov 21, 2019 at 09:40:05AM +0100, Ahmad Fatoum wrote:
>> Both the STM32 and i.MX7 remote proc drivers populate the .stop member
>> in the struct rproc, but it's not used anywhere.
> 
> The .stop member in struct rproc is introduced in this patch.

Indeed. I was referring to the stop member in the ops struct, which is
a so-far unused function pointer.

>>  	ret = rproc_start(rproc, &fw);
>> +	if (ret == 0)
>> +		rproc->stop = PARAM_TRISTATE_FALSE;
> 
> Can we use positive logic here? "Status Stopped is false" is harder to
> read than just "running" or "started".

Naming it .stop emphasizes the fact that it's only meant to stop execution,
not start it. See below.


>> +	return stop(rproc);
>> +}
> 
> I would assume that when I can stop the remote processor with this
> parameter I should be able to start it here as well, no?

Which firmware would the processor execute when started via parameter?
I see no benefit in powering up the co-processor without specifying a
firmware image.

Thoughts?
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] remoteproc: add .stop device parameter for stopping remote processor

[Ahmad Fatoum]

Hello Sascha,

On 11/25/19 9:28 AM, Sascha Hauer wrote:
> I would assume that when I can stop the remote processor with this
> parameter I should be able to start it here as well, no?

I've yet to think some more about this, but could you merge the first
two commits now?

Cheers
Ahmad


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] remoteproc: add .stop device parameter for stopping remote processor

[Sascha Hauer]

On Wed, Dec 04, 2019 at 02:07:48PM +0100, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 11/25/19 9:28 AM, Sascha Hauer wrote:
> > I would assume that when I can stop the remote processor with this
> > parameter I should be able to start it here as well, no?
> 
> I've yet to think some more about this, but could you merge the first
> two commits now?

Did that.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox