On Thu, Jun 05, 2025 at 09:37:42PM +0530, Wasim Nazir wrote:
> On Thu, Jun 05, 2025 at 10:23:51AM -0500, Bjorn Andersson wrote:
> > The change that renamed the driver from "adsp" to "pas" didn't change
> > any of the implementation. The result is an aesthetic eyesore, and
> > confusing to many.
> >
> > Conclude the rename of the driver, by updating function, structures and
> > variable names to match what the driver actually is. The "Hexagon v5" is
> > also dropped from the name and Kconfig, as this isn't correct either.
> >
> > No functional change.
> >
> > Fixes: 9e004f97161d ("remoteproc: qcom: Rename Hexagon v5 PAS driver")
> > Signed-off-by: Bjorn Andersson <bjorn.anders...@oss.qualcomm.com>
> > ---
> > drivers/remoteproc/Kconfig | 11 +-
> > drivers/remoteproc/qcom_q6v5_adsp.c | 46 +--
> > drivers/remoteproc/qcom_q6v5_pas.c | 617
> > ++++++++++++++++++------------------
> > 3 files changed, 334 insertions(+), 340 deletions(-)
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index
> > 83962a114dc9fdb3260e6e922602f2da53106265..4a1e469acaf139334686af1eb962ce9420c6ddb1
> > 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -214,7 +214,7 @@ config QCOM_Q6V5_MSS
> > handled by QCOM_Q6V5_PAS driver.
> >
> > config QCOM_Q6V5_PAS
> > - tristate "Qualcomm Hexagon v5 Peripheral Authentication Service support"
> > + tristate "Qualcomm Peripheral Authentication Service support"
> > depends on OF && ARCH_QCOM
> > depends on QCOM_SMEM
> > depends on RPMSG_QCOM_SMD || RPMSG_QCOM_SMD=n
> > @@ -229,11 +229,10 @@ config QCOM_Q6V5_PAS
> > select QCOM_RPROC_COMMON
> > select QCOM_SCM
> > help
> > - Say y here to support the TrustZone based Peripheral Image Loader
> > - for the Qualcomm Hexagon v5 based remote processors. This is commonly
> > - used to control subsystems such as ADSP (Audio DSP),
> > - CDSP (Compute DSP), MPSS (Modem Peripheral SubSystem), and
> > - SLPI (Sensor Low Power Island).
> > + Say y here to support the TrustZone based Peripheral Image Loader for
> > + the Qualcomm based remote processors. This is commonly used to
>
> Maybe "Qualcomm remote processors"?
>
That sounds better, thanks.
> > + control subsystems such as ADSP (Audio DSP), CDSP (Compute DSP), MPSS
> > + (Modem Peripheral SubSystem), and SLPI (Sensor Low Power Island).
> >
> > config QCOM_Q6V5_WCSS
> > tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader"
> > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
> > b/drivers/remoteproc/qcom_q6v5_adsp.c
> > index
> > 94af77baa7a1c5096f0663260c07a297c6bedd17..613826e0d7eff1712ca31ea102adef4f62d10f38
> > 100644
> > --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> > @@ -77,7 +77,7 @@ struct adsp_pil_data {
> > const char *load_state;
> > };
> >
> > -struct qcom_adsp {
> > +struct qcom_pas {
>
> Any reason to change in this file?
>
Wow, no of course not. I asked my editor to rename the symbols and
missed the fact that it changed the names of the symbols in both files,
that's weird.
Thank you for spotting that.
[..]
> > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
> > b/drivers/remoteproc/qcom_q6v5_pas.c
> > index
> > b306f223127c452f8f2d85aa0fc98db2d684feae..b0fc372ff0a9e032d784b1a4403ffeea5d0f9a00
> > 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pas.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> > @@ -1,6 +1,6 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > /*
> > - * Qualcomm ADSP/SLPI Peripheral Image Loader for MSM8974 and MSM8996
> > + * Qualcomm Peripahal Authentication Service remoteproc driver
> > *
> > * Copyright (C) 2016 Linaro Ltd
> > * Copyright (C) 2014 Sony Mobile Communications AB
> > @@ -35,7 +35,7 @@
> >
> > #define MAX_ASSIGN_COUNT 3
> >
> > -struct adsp_data {
> > +struct qcom_pas_data {
> > int crash_reason_smem;
> > const char *firmware_name;
> > const char *dtb_firmware_name;
> > @@ -60,7 +60,7 @@ struct adsp_data {
> > int region_assign_vmid;
> > };
> >
> > -struct qcom_adsp {
> > +struct qcom_pas {
> > struct device *dev;
> > struct rproc *rproc;
> >
> > @@ -119,36 +119,37 @@ struct qcom_adsp {
> > struct qcom_scm_pas_metadata dtb_pas_metadata;
> > };
> >
> > -static void adsp_segment_dump(struct rproc *rproc, struct
> > rproc_dump_segment *segment,
> > - void *dest, size_t offset, size_t size)
> > +static void qcom_pas_segment_dump(struct rproc *rproc,
> > + struct rproc_dump_segment *segment,
> > + void *dest, size_t offset, size_t size)
> > {
> > - struct qcom_adsp *adsp = rproc->priv;
> > + struct qcom_pas *pas = rproc->priv;
> > int total_offset;
> >
> > - total_offset = segment->da + segment->offset + offset - adsp->mem_phys;
> > - if (total_offset < 0 || total_offset + size > adsp->mem_size) {
> > - dev_err(adsp->dev,
> > + total_offset = segment->da + segment->offset + offset - pas->mem_phys;
> > + if (total_offset < 0 || total_offset + size > pas->mem_size) {
> > + dev_err(pas->dev,
> > "invalid copy request for segment %pad with offset %zu
> > and size %zu)\n",
> > &segment->da, offset, size);
> > memset(dest, 0xff, size);
> > return;
> > }
> >
> > - memcpy_fromio(dest, adsp->mem_region + total_offset, size);
> > + memcpy_fromio(dest, pas->mem_region + total_offset, size);
> > }
> >
> > -static void adsp_minidump(struct rproc *rproc)
> > +static void qcom_pas_minidump(struct rproc *rproc)
> > {
> > - struct qcom_adsp *adsp = rproc->priv;
> > + struct qcom_pas *pas = rproc->priv;
> >
> > if (rproc->dump_conf == RPROC_COREDUMP_DISABLED)
> > return;
> >
> > - qcom_minidump(rproc, adsp->minidump_id, adsp_segment_dump);
> > + qcom_minidump(rproc, pas->minidump_id, qcom_pas_segment_dump);
> > }
> >
> > -static int adsp_pds_enable(struct qcom_adsp *adsp, struct device **pds,
> > - size_t pd_count)
> > +static int qcom_pas_pds_enable(struct qcom_pas *pas, struct device **pds,
> > + size_t pd_count)
> > {
> > int ret;
> > int i;
> > @@ -174,8 +175,8 @@ static int adsp_pds_enable(struct qcom_adsp *adsp,
> > struct device **pds,
> > return ret;
> > };
> >
> > -static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
> > - size_t pd_count)
> > +static void qcom_pas_pds_disable(struct qcom_pas *pas, struct device **pds,
> > + size_t pd_count)
> > {
> > int i;
> >
> > @@ -185,65 +186,65 @@ static void adsp_pds_disable(struct qcom_adsp *adsp,
> > struct device **pds,
> > }
> > }
> >
> > -static int adsp_shutdown_poll_decrypt(struct qcom_adsp *adsp)
> > +static int qcom_pas_shutdown_poll_decrypt(struct qcom_pas *pas)
> > {
> > unsigned int retry_num = 50;
> > int ret;
> >
> > do {
> > msleep(ADSP_DECRYPT_SHUTDOWN_DELAY_MS);
>
> Do you want to change the macro too?
>
That would make sense, thanks for spotting that!
> > - ret = qcom_scm_pas_shutdown(adsp->pas_id);
> > + ret = qcom_scm_pas_shutdown(pas->pas_id);
> > } while (ret == -EINVAL && --retry_num);
> >
> > return ret;
> > }
> >
> > -static int adsp_unprepare(struct rproc *rproc)
> > +static int qcom_pas_unprepare(struct rproc *rproc)
> > {
> > - struct qcom_adsp *adsp = rproc->priv;
> > + struct qcom_pas *pas = rproc->priv;
> >
> > /*
> > - * adsp_load() did pass pas_metadata to the SCM driver for storing
> > + * pas_load() did pass pas_metadata to the SCM driver for storing
>
> Don't see pas_load() API in this file. Please check if you are referring to
> qcom_pas_load().
>
Yes, I refer to the qcom_pas_load().
[..]
> > -static int adsp_pds_attach(struct device *dev, struct device **devs,
> > - char **pd_names)
> > +static int qcom_pas_pds_attach(struct device *dev, struct device **devs,
> > char **pd_names)
>
> Can you check the indentation to 80 characters?
>
We prefer 80 characters, but we allow up to 100 if it makes the code
cleaner. So not breaking this line was intentional...
> > {
> > size_t num_pds = 0;
> > int ret;
> > @@ -528,10 +527,9 @@ static int adsp_pds_attach(struct device *dev, struct
> > device **devs,
> > return ret;
> > };
> >
> > -static void adsp_pds_detach(struct qcom_adsp *adsp, struct device **pds,
> > - size_t pd_count)
> > +static void qcom_pas_pds_detach(struct qcom_pas *pas, struct device **pds,
> > size_t pd_count)
>
> Same indentation needed here.
>
91 characters here, and I find it looks better given the "logical"
relation between pds and pd_count otherwise being split across two
lines.
Many thanks for the review, Wasim!
Regards,
Bjorn
