From: Peng Fan <[email protected]>
remote processor may support:
- firmware recovery with help from main processor
- self recovery without help from main processor
- iommu
- etc
Introduce rproc features could simplify code to avoid adding more bool
flags and let us optimize current code.
Signed-off-by: Peng Fan <[email protected]>
---
V2:
New
include/linux/remoteproc.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 93a1d0050fbc..51edaf80692c 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -417,6 +417,7 @@ struct rproc_ops {
* has attached to it
* @RPROC_DETACHED: device has been booted by another entity and waiting
* for the core to attach to it
+ * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self recovery
* @RPROC_LAST: just keep this one at the end
*
* Please note that the values of these states are used as indices
@@ -489,6 +490,11 @@ struct rproc_dump_segment {
loff_t offset;
};
+enum rproc_features {
+ RPROC_FEAT_ATTACH_RECOVERY = 0,
+ RPROC_MAX_FEATURES = 32,
+};
+
/**
* struct rproc - represents a physical remote processor device
* @node: list node of this rproc object
@@ -530,6 +536,7 @@ struct rproc_dump_segment {
* @elf_machine: firmware ELF machine
* @cdev: character device of the rproc
* @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @features: indicate remoteproc features
*/
struct rproc {
struct list_head node;
@@ -570,8 +577,19 @@ struct rproc {
u16 elf_machine;
struct cdev cdev;
bool cdev_put_on_release;
+ DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
};
+static inline bool rproc_has_feature(struct rproc *rproc, unsigned int feature)
+{
+ return test_bit(feature, rproc->features);
+}
+
+static inline void rproc_set_feature(struct rproc *rproc, unsigned int feature)
+{
+ set_bit(feature, rproc->features);
+}
+
/**
* struct rproc_subdev - subdevice tied to a remoteproc
* @node: list node related to the rproc subdevs list
--
2.30.0
On Tue 08 Mar 00:48 CST 2022, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> remote processor may support:
> - firmware recovery with help from main processor
> - self recovery without help from main processor
> - iommu
> - etc
>
> Introduce rproc features could simplify code to avoid adding more bool
> flags and let us optimize current code.
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
>
> V2:
> New
>
> include/linux/remoteproc.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
This is the API that other parts of the kernel use to interact with a
struct rproc, what feature flags do you have a need for other parts of
the kernel to be able to query?
> index 93a1d0050fbc..51edaf80692c 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -417,6 +417,7 @@ struct rproc_ops {
> * has attached to it
> * @RPROC_DETACHED: device has been booted by another entity and waiting
> * for the core to attach to it
> + * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self recovery
This seems to belong in the other patch...
> * @RPROC_LAST: just keep this one at the end
> *
> * Please note that the values of these states are used as indices
> @@ -489,6 +490,11 @@ struct rproc_dump_segment {
> loff_t offset;
> };
>
> +enum rproc_features {
> + RPROC_FEAT_ATTACH_RECOVERY = 0,
No need to specify that this is bit 0, and the enum will do that for you.
> + RPROC_MAX_FEATURES = 32,
You're using DECLARE_BITMAP() so why 32?
Regards,
Bjorn
> +};
> +
> /**
> * struct rproc - represents a physical remote processor device
> * @node: list node of this rproc object
> @@ -530,6 +536,7 @@ struct rproc_dump_segment {
> * @elf_machine: firmware ELF machine
> * @cdev: character device of the rproc
> * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @features: indicate remoteproc features
> */
> struct rproc {
> struct list_head node;
> @@ -570,8 +577,19 @@ struct rproc {
> u16 elf_machine;
> struct cdev cdev;
> bool cdev_put_on_release;
> + DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> };
>
> +static inline bool rproc_has_feature(struct rproc *rproc, unsigned int feature)
> +{
> + return test_bit(feature, rproc->features);
> +}
> +
> +static inline void rproc_set_feature(struct rproc *rproc, unsigned int feature)
> +{
> + set_bit(feature, rproc->features);
> +}
> +
> /**
> * struct rproc_subdev - subdevice tied to a remoteproc
> * @node: list node related to the rproc subdevs list
> --
> 2.30.0
>
> Subject: Re: [PATCH V2 1/2] remoteproc: introduce rproc features
>
> On Tue 08 Mar 00:48 CST 2022, Peng Fan (OSS) wrote:
>
> > From: Peng Fan <[email protected]>
> >
> > remote processor may support:
> > - firmware recovery with help from main processor
> > - self recovery without help from main processor
> > - iommu
> > - etc
> >
> > Introduce rproc features could simplify code to avoid adding more bool
> > flags and let us optimize current code.
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> >
> > V2:
> > New
> >
> > include/linux/remoteproc.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>
> This is the API that other parts of the kernel use to interact with a struct rproc,
> what feature flags do you have a need for other parts of the kernel to be able
> to query?
I no need other parts of kernel to query rproc features.
In my V1 patch to support i.MX8QM/QXP self recovery, I introduced a new bool
in "struct rproc", but it is not preferred. So I think to use a enum flag, after
looking into struct rproc, I see the possibility to use one API rproc_has_feature
to simplify code, such as move has_iommu into
rproc_has_feature(rproc, RPROC_FEAT_IOMMU), and in future, when need to
add new flag/capability of rproc, we could reuse the rproc_has_feature API.
>
> > index 93a1d0050fbc..51edaf80692c 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -417,6 +417,7 @@ struct rproc_ops {
> > * has attached to it
> > * @RPROC_DETACHED: device has been booted by another entity and
> waiting
> > * for the core to attach to it
> > + * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self
> > + recovery
>
> This seems to belong in the other patch...
I'll move this to patch 2/2
>
> > * @RPROC_LAST: just keep this one at the end
> > *
> > * Please note that the values of these states are used as indices @@
> > -489,6 +490,11 @@ struct rproc_dump_segment {
> > loff_t offset;
> > };
> >
> > +enum rproc_features {
> > + RPROC_FEAT_ATTACH_RECOVERY = 0,
>
> No need to specify that this is bit 0, and the enum will do that for you.
sure
>
> > + RPROC_MAX_FEATURES = 32,
>
> You're using DECLARE_BITMAP() so why 32?
Just wanna to limit max features to one int, I may mis use this,
will fix in new version.
Thanks,
Peng.
>
> Regards,
> Bjorn
>
> > +};
> > +
> > /**
> > * struct rproc - represents a physical remote processor device
> > * @node: list node of this rproc object @@ -530,6 +536,7 @@ struct
> > rproc_dump_segment {
> > * @elf_machine: firmware ELF machine
> > * @cdev: character device of the rproc
> > * @cdev_put_on_release: flag to indicate if remoteproc should be
> > shutdown on @char_dev release
> > + * @features: indicate remoteproc features
> > */
> > struct rproc {
> > struct list_head node;
> > @@ -570,8 +577,19 @@ struct rproc {
> > u16 elf_machine;
> > struct cdev cdev;
> > bool cdev_put_on_release;
> > + DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> > };
> >
> > +static inline bool rproc_has_feature(struct rproc *rproc, unsigned
> > +int feature) {
> > + return test_bit(feature, rproc->features); }
> > +
> > +static inline void rproc_set_feature(struct rproc *rproc, unsigned
> > +int feature) {
> > + set_bit(feature, rproc->features);
> > +}
> > +
> > /**
> > * struct rproc_subdev - subdevice tied to a remoteproc
> > * @node: list node related to the rproc subdevs list
> > --
> > 2.30.0
> >