On Fri 27 Jul 08:20 PDT 2018, Sibi Sankar wrote:
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
[..]
> +static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr, size_t len,
> + void *priv)
> +{
> + int ret = 0;
> + struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> + static u32 pending_mask;
I dislike that this is a static variable. And it tracks the segments
that has already been dumped, i.e. the !pending.
> +
> + /* Unlock mba before copying segments */
> + if (!qproc->mba_loaded)
> + ret = q6v5_mba_load(qproc);
> +
> + if (!ptr || ret)
> + memset(priv, 0xff, len);
> + else
> + memcpy(priv, ptr, len);
> +
> + pending_mask++;
This is a "count" and not a "mask".
I can see a few different cases where one would like to be able to pass
custom data/information from the segment-registration to the dump
function. So how about adding a "void *priv" to the dump segment.
For this particular case we could typecast segment->priv to an unsigned
long (as this is always the same size) and use that as a bitmask, which
we use to update pending_mask.
> + if (pending_mask == qproc->valid_mask) {
> + if (qproc->mba_loaded)
> + q6v5_mba_reclaim(qproc);
> + pending_mask = 0;
> + }
I think it would be cleaner to reset pending_mask in the start function,
and then return early in this function when we have dumped all the
segments.
If so can pending_mask == 0 and pending_mask == all be the triggers for
loading and reclaiming the mba? So we don't have two different trackers
for this?
> +}
> +
Regards,
Bjorn
