| Message ID | 20220308064821.2154-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V2,1/2] remoteproc: introduce rproc features | expand |
On Tue 08 Mar 00:48 CST 2022, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > 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 <peng.fan@nxp.com> > --- > > 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 <peng.fan@nxp.com> > > > > 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 <peng.fan@nxp.com> > > --- > > > > 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 > >
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