Messages in this thread | | | Date | Sun, 7 Oct 2018 23:45:05 -0700 | From | Bjorn Andersson <> | Subject | Re: [PATCH v3 4/6] remoteproc: qcom: q6v5-pil: Add custom dump function for modem |
| |
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
| |