On Mon, Aug 05, 2019 at 03:16:41PM +0200, Etienne Carriere wrote:
> On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.ho...@arm.com> wrote:
> >
> > SCMI v2.0 adds support for "FastChannel" which do not use a message
> > header as they are specialized for a single message.
> >
> > Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> > commands are supported over fastchannels. As they are optional, they
> > need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> > Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
> >
> > Add support for making use of these fastchannels.
> >
> > Cc: Ionela Voinescu <ionela.voine...@arm.com>
> > Cc: Chris Redpath <chris.redp...@arm.com>
> > Cc: Quentin Perret <quentin.per...@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.ho...@arm.com>
> > ---
> > drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
> > 1 file changed, 100 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/perf.c
> > b/drivers/firmware/arm_scmi/perf.c
> > index 6cce3e82e81e..b9144efbd6fb 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -8,6 +8,7 @@
> > #include <linux/bits.h>
> > #include <linux/of.h>
> > #include <linux/io.h>
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_opp.h>
> > #include <linux/sort.h>
> > @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle
> > *handle, u32 domain,
> > return ret;
> > }
> >
> > -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32
> > domain,
> > +#define SCMI_PERF_FC_RING_DB(doorbell, w) \
>
> Suggest to reformat into a macro style:
> #define SCMI_PERF_FC_RING_DB(doorbell, w) \
> do { \
> (...) \
> } while (0)
>
Sure I can try that.
> > +{ \
> > + u##w val = 0; \
> > + struct scmi_fc_db_info *db = doorbell; \
> > + \
> > + if ((db)->mask) \
>
> remove parentheses. I would say, could simply remove `if (db->mask)` here.
Ah, missed to drop this one. We can avoid reading the value if mask = 0,
so I prefer to keep it.
>
> > + val = ioread##w(db->addr) & db->mask; \
> > + iowrite##w((u##w)db->set | val, db->addr); \
> > +}
> > +
> > +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> > +{
> > + if (!db || !db->addr)
> > + return;
> > +
> > + if (db->width == 1)
> > + SCMI_PERF_FC_RING_DB(db, 8)
> > + else if (db->width == 2)
> > + SCMI_PERF_FC_RING_DB(db, 16)
> > + else if (db->width == 4)
> > + SCMI_PERF_FC_RING_DB(db, 32)
> > + else /* db->width == 8 */
> > +#ifdef CONFIG_64BIT
> > + SCMI_PERF_FC_RING_DB(db, 64)
> > +#else
> > + {
> > + u64 val = 0;
> > +
> > + if (db->mask)
> > + val = ioread64_hi_lo(db->addr) & db->mask;
> > + iowrite64_hi_lo(db->set, db->addr);
>
> Is `value` missing here?
> Why not using SCMI_PERF_FC_RING_DB(db, 64) here?
>
I am still using it. Just if CONFIG_64BIT is enabled and io{read,write}64
are defined. ARM32/MIPS and other 32-bit platform build might fail
otherwise. I don't want to restrict SCMI to 64-bit platforms only.
--
Regards,
Sudeep
