Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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