On Mon, Nov 03, 2025 at 08:59:29PM +0530, Deepak Kumar Singh wrote: > From: Chris Lew <[email protected]> > > Some remoteproc need smp2p v2 support, mirror the version written by the > remote if the remote supports v2.
I don't think they _need_ v2 support. The subsystem might implement v2 and only support v2... > This is needed if the remote does not have backwards compatibility > with v1. I guess this retroactively amends the previous sentence to make it valid? Please rewrite these two sentences. > And reset entry last value on SSR for smp2p v2. The first two sentences described a problem and a "solution" to that problem, here you're just throwing in a fact. Please document what version 2 actually is and make it clear why resetting "entry last value on SSR". > > Signed-off-by: Chris Lew <[email protected]> > Signed-off-by: Deepak Kumar Singh <[email protected]> > --- > drivers/soc/qcom/smp2p.c | 41 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 38 insertions(+), 3 deletions(-) > > diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c > index 39628df36745..c35ca7535c14 100644 > --- a/drivers/soc/qcom/smp2p.c > +++ b/drivers/soc/qcom/smp2p.c > @@ -36,6 +36,10 @@ > * The driver uses the Linux GPIO and interrupt framework to expose a virtual > * GPIO for each outbound entry and a virtual interrupt controller for each > * inbound entry. > + * > + * Driver supports two versions: > + * V1 - For processor that start after local host > + * V2 - For processor that start in early boot sequence > */ > > #define SMP2P_MAX_ENTRY 16 > @@ -50,11 +54,12 @@ > > #define ONE 1 > #define TWO 2 > +#define MAX_VERSION TWO > > /** > * struct smp2p_smem_item - in memory communication structure > * @magic: magic number > - * @version: version - must be 1 > + * @version: version > * @features: features flag - currently unused > * @local_pid: processor id of sending end > * @remote_pid: processor id of receiving end > @@ -183,14 +188,23 @@ static void qcom_smp2p_kick(struct qcom_smp2p *smp2p) > static bool qcom_smp2p_check_ssr(struct qcom_smp2p *smp2p) > { > struct smp2p_smem_item *in = smp2p->in; > + struct smp2p_entry *entry; > bool restart; > > if (!smp2p->ssr_ack_enabled) > return false; > > restart = in->flags & BIT(SMP2P_FLAGS_RESTART_DONE_BIT); > + restart = restart != smp2p->ssr_ack; This is hard to read, please try to avoid the immediate reassignment. > + if (restart && in->version > ONE) { > + list_for_each_entry(entry, &smp2p->inbound, node) { > + if (!entry->value) > + continue; > + entry->last_value = 0; Why do we only do this for version 2+? > + } > + } > > - return restart != smp2p->ssr_ack; > + return restart; > } > > static void qcom_smp2p_do_ssr_ack(struct qcom_smp2p *smp2p) > @@ -225,6 +239,20 @@ static void qcom_smp2p_negotiate(struct qcom_smp2p > *smp2p) > } > } > > +static int qcom_smp2p_in_version(struct qcom_smp2p *smp2p) > +{ > + unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND]; > + unsigned int pid = smp2p->remote_pid; > + struct smp2p_smem_item *in; > + size_t size; > + > + in = qcom_smem_get(pid, smem_id, &size); > + if (IS_ERR(in)) > + return 0; > + > + return in->version; > +} > + > static void qcom_smp2p_start_in(struct qcom_smp2p *smp2p) > { > unsigned int smem_id = smp2p->smem_items[SMP2P_INBOUND]; > @@ -522,6 +550,7 @@ static int qcom_smp2p_alloc_outbound_item(struct > qcom_smp2p *smp2p) > struct smp2p_smem_item *out; > unsigned smem_id = smp2p->smem_items[SMP2P_OUTBOUND]; > unsigned pid = smp2p->remote_pid; > + u8 in_version; > int ret; > > ret = qcom_smem_alloc(pid, smem_id, sizeof(*out)); > @@ -543,12 +572,18 @@ static int qcom_smp2p_alloc_outbound_item(struct > qcom_smp2p *smp2p) > out->valid_entries = 0; > out->features = SMP2P_ALL_FEATURES; > > + in_version = qcom_smp2p_in_version(smp2p); This is a bit obfuscated in my view, and doesn't seem complete. We're calling qcom_smp2p_alloc_outbound_item() during probe(), at which time any non-early booted subsystem will yet to have been launched and hence they haven't allocated their SMP2P SMEM item. So in_version will be 0, which is less than 2, so therefor we're running version 1. If the subsystem is then brought out of reset and it implements version 2 (we intended it to be launched by bootloader, but it wasn't - this shouldn't be a problem), we will have a version "mismatch". Upon first interrupt from the remote we will determine in qcom_smp2p_negotiate() that we're version 1 and they are version 2, so we will not complete the negotiation - and thereby not deliver interrupts. > + if (in_version > MAX_VERSION) { > + dev_err(smp2p->dev, "Unsupported smp2p version\n"); I think we can afford ourself to add "...: %d\n", in_version); here. It would make the error print directly actionable the day we hit it (or make it obvious if we hit this error due to a bogus in_version). Regards, Bjorn > + return -EINVAL; > + } > + > /* > * Make sure the rest of the header is written before we validate the > * item by writing a valid version number. > */ > wmb(); > - out->version = 1; > + out->version = (in_version) ? in_version : 1; > > qcom_smp2p_kick(smp2p); > > -- > 2.34.1 >
