> 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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> >
> > 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
> >
