2023-02-13 18:53:34

by Melody Olvera

[permalink] [raw]
Subject: [PATCH 0/9] remoteproc: qcom_q6v5_pas: Add support for QDU1000/QRU1000 mpss

This patchset adds support for the mpss found in the QDU1000 and QRU1000
SoCs.

The mpss boot process now supports late attach for an already running
mpss. For this, it uses an RMB register space to perform a handshake
with the mpss for the late attach process. This is implemented in the
patches below. The patches also address issues with split binary
detection to support loading of split binaries more robustly.

Gokul Krishna Krishnakumar (1):
soc: qcom: mdt_loader: Enhance split binary detection

Melody Olvera (8):
dt-bindings: firmware: qcom,scm: Update QDU1000/QRU1000 compatible
dt-bindings: mailbox: qcom-ipcc: Add compatible for QDU1000/QRU1000
dt-bindings: soc: qcom: aoss: Document power-domain-cells for aoss
dt-bindings: soc: qcom: aoss: Document QDU1000/QRU1000 compatible
dt-bindings: remoteproc: mpss: Document QDU1000/QRU1000 mpss devices
remoteproc: qcom: q6v5: Add support for q6 rmb registers
remoteproc: qcom_q6v5_pas: Add support to attach a DSP
remoteproc: qcom_q6v5_pas: Add QDU1000/QRU1000 mpss compatible & data

.../bindings/firmware/qcom,scm.yaml | 1 +
.../bindings/mailbox/qcom-ipcc.yaml | 1 +
.../bindings/remoteproc/qcom,qdu1000-pas.yaml | 127 ++++++++++++++++++
.../bindings/soc/qcom/qcom,aoss-qmp.yaml | 5 +
drivers/remoteproc/qcom_q6v5.c | 9 ++
drivers/remoteproc/qcom_q6v5.h | 8 ++
drivers/remoteproc/qcom_q6v5_pas.c | 126 ++++++++++++++++-
drivers/soc/qcom/mdt_loader.c | 64 +++++----
8 files changed, 311 insertions(+), 30 deletions(-)
create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml


base-commit: 09e41676e35ab06e4bce8870ea3bf1f191c3cb90
--
2.25.1



2023-02-13 18:53:39

by Melody Olvera

[permalink] [raw]
Subject: [PATCH 5/9] dt-bindings: remoteproc: mpss: Document QDU1000/QRU1000 mpss devices

This documents the compatible for the component used to boot the
MPSS on the QDU1000 and QRU1000 SoCs.

The QDU1000 and QRU1000 mpss boot process now requires the specification
of an RMB register space to complete the handshake needed to start or
attach the mpss.

Signed-off-by: Melody Olvera <[email protected]>
---
.../bindings/remoteproc/qcom,qdu1000-pas.yaml | 127 ++++++++++++++++++
1 file changed, 127 insertions(+)
create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
new file mode 100644
index 000000000000..eb6ade984778
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
@@ -0,0 +1,127 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,qdu1000-pas.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm QDU1000 Peripheral Authentication Service
+
+maintainers:
+ - Melody Olvera <[email protected]>
+
+description:
+ Qualcomm QDU1000 SoC Peripheral Authentication Service loads and boots firmware
+ on the Qualcomm DSP Hexagon cores.
+
+properties:
+ compatible:
+ enum:
+ - qcom,qdu1000-mpss-pas
+
+ reg:
+ maxItems: 2
+
+ clocks:
+ items:
+ - description: XO clock
+
+ clock-names:
+ items:
+ - const: xo
+
+ qcom,qmp:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: Reference to the AOSS side-channel message RAM.
+
+ smd-edge: false
+
+ firmware-name:
+ $ref: /schemas/types.yaml#/definitions/string-array
+ items:
+ - description: Firmware name of the Hexagon core
+ - description: Firmware name of the Hexagon Devicetree
+
+ memory-region:
+ items:
+ - description: Memory region for main Firmware authentication
+ - description: Memory region for Devicetree Firmware authentication
+ - description: DSM Memory region
+
+ interrupts:
+ minItems: 6
+
+ interrupt-names:
+ minItems: 6
+
+ interconnects:
+ minItems: 1
+
+ power-domains:
+ items:
+ - description: CX power domain
+ - description: MSS power domain
+
+ power-domain-names:
+ items:
+ - const: cx
+ - const: mss
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - $ref: /schemas/remoteproc/qcom,pas-common.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,rpmh.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/mailbox/qcom-ipcc.h>
+
+ remoteproc@4080000 {
+ compatible = "qcom,qdu1000-mpss-pas";
+ reg = <0x4080000 0x4040>,
+ <0x4180000 0x1000>;
+
+ clocks = <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "xo";
+
+ interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
+ <&smp2p_modem_in 0 IRQ_TYPE_EDGE_RISING>,
+ <&smp2p_modem_in 1 IRQ_TYPE_EDGE_RISING>,
+ <&smp2p_modem_in 2 IRQ_TYPE_EDGE_RISING>,
+ <&smp2p_modem_in 3 IRQ_TYPE_EDGE_RISING>
+ <&smp2p_modem_in 7 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "wdog", "fatal", "ready", "handover",
+ "stop-ack", "shutdown-ack";
+
+ memory-region = <&mpss_mem>, <&dtb_mpss_mem>, <&mpss_dsm_mem>;
+
+ firmware-name = "modem.mdt",
+ "modem_dtb.mdt";
+
+ power-domains = <&rpmhpd QDU1000_CX>,
+ <&rpmhpd QDU1000_MSS>;
+ power-domain-names = "cx", "mss";
+
+ interconnects = <&mc_virt MASTER_LLCC &mc_virt SLAVE_EBI1>;
+
+ qcom,qmp = <&aoss_qmp>;
+
+ qcom,smem-states = <&smp2p_adsp_out 0>;
+ qcom,smem-state-names = "stop";
+
+ glink-edge {
+ interrupts-extended = <&ipcc IPCC_CLIENT_MPSS
+ IPCC_MPROC_SIGNAL_GLINK_QMP
+ IRQ_TYPE_EDGE_RISING>;
+ mboxes = <&ipcc IPCC_CLIENT_MPSS IPCC_MPROC_SIGNAL_GLINK_QMP>;
+
+ label = "modem";
+ qcom,remote-pid = <2>;
+
+ };
+ };
--
2.25.1


2023-02-13 18:53:43

by Melody Olvera

[permalink] [raw]
Subject: [PATCH 7/9] remoteproc: qcom: q6v5: Add support for q6 rmb registers

When attaching a running Q6, the remoteproc driver needs a way
to communicate with the Q6 using rmb registers, so allow the
rmb register to be gotten from the device tree if present.

Signed-off-by: Melody Olvera <[email protected]>
---
drivers/remoteproc/qcom_q6v5.c | 9 +++++++++
drivers/remoteproc/qcom_q6v5.h | 8 ++++++++
2 files changed, 17 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 192c7aa0e39e..e8c6be70ebfd 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -254,6 +254,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
void (*handover)(struct qcom_q6v5 *q6v5))
{
int ret;
+ struct resource *res;

q6v5->rproc = rproc;
q6v5->dev = &pdev->dev;
@@ -263,6 +264,14 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
init_completion(&q6v5->start_done);
init_completion(&q6v5->stop_done);

+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (res) {
+ q6v5->rmb_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(q6v5->rmb_base))
+ q6v5->rmb_base = NULL;
+ } else
+ q6v5->rmb_base = NULL;
+
q6v5->wdog_irq = platform_get_irq_byname(pdev, "wdog");
if (q6v5->wdog_irq < 0)
return q6v5->wdog_irq;
diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
index 5a859c41896e..95824d5b64ce 100644
--- a/drivers/remoteproc/qcom_q6v5.h
+++ b/drivers/remoteproc/qcom_q6v5.h
@@ -7,6 +7,12 @@
#include <linux/completion.h>
#include <linux/soc/qcom/qcom_aoss.h>

+#define RMB_BOOT_WAIT_REG 0x8
+#define RMB_BOOT_CONT_REG 0xC
+#define RMB_Q6_BOOT_STATUS_REG 0x10
+
+#define RMB_POLL_MAX_TIMES 250
+
struct icc_path;
struct rproc;
struct qcom_smem_state;
@@ -16,6 +22,8 @@ struct qcom_q6v5 {
struct device *dev;
struct rproc *rproc;

+ void __iomem *rmb_base;
+
struct qcom_smem_state *state;
struct qmp *qmp;

--
2.25.1


2023-02-13 18:53:45

by Melody Olvera

[permalink] [raw]
Subject: [PATCH 6/9] soc: qcom: mdt_loader: Enhance split binary detection

From: Gokul Krishna Krishnakumar <[email protected]>

When booting with split binaries, it is possible that the
mdt loader misdetects if a binary is split and only loads
one of the segments, so enhance the detection of the split
binaries to ensure the entirety of the firmware is loaded.

Signed-off-by: Gokul Krishna Krishnakumar <[email protected]>
Signed-off-by: Melody Olvera <[email protected]>
---
drivers/soc/qcom/mdt_loader.c | 64 +++++++++++++++++++----------------
1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 33dd8c315eb7..3aadce299c02 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
return true;
}

+static bool qcom_mdt_bins_are_split(const struct firmware *fw)
+{
+ const struct elf32_phdr *phdrs;
+ const struct elf32_hdr *ehdr;
+ uint64_t seg_start, seg_end;
+ int i;
+
+ ehdr = (struct elf32_hdr *)fw->data;
+ phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+ for (i = 0; i < ehdr->e_phnum; i++) {
+ seg_start = phdrs[i].p_offset;
+ seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
+ if (seg_start > fw->size || seg_end > fw->size)
+ return true;
+ }
+
+ return false;
+}
+
static ssize_t mdt_load_split_segment(void *ptr, const struct elf32_phdr *phdrs,
unsigned int segment, const char *fw_name,
struct device *dev)
@@ -167,23 +187,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
/* Copy ELF header */
memcpy(data, fw->data, ehdr_size);

- if (ehdr_size + hash_size == fw->size) {
- /* Firmware is split and hash is packed following the ELF header */
- hash_offset = phdrs[0].p_filesz;
- memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
- } else if (phdrs[hash_segment].p_offset + hash_size <= fw->size) {
- /* Hash is in its own segment, but within the loaded file */
+
+ if (qcom_mdt_bins_are_split(fw)) {
+ ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
+ } else {
hash_offset = phdrs[hash_segment].p_offset;
memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
- } else {
- /* Hash is in its own segment, beyond the loaded file */
- ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
- if (ret) {
- kfree(data);
- return ERR_PTR(ret);
- }
}
-
*data_len = ehdr_size + hash_size;

return data;
@@ -270,6 +280,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
phys_addr_t min_addr = PHYS_ADDR_MAX;
ssize_t offset;
bool relocate = false;
+ bool is_split;
void *ptr;
int ret = 0;
int i;
@@ -277,6 +288,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
if (!fw || !mem_region || !mem_phys || !mem_size)
return -EINVAL;

+ is_split = qcom_mdt_bins_are_split(fw);
ehdr = (struct elf32_hdr *)fw->data;
phdrs = (struct elf32_phdr *)(ehdr + 1);

@@ -330,22 +342,16 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,

ptr = mem_region + offset;

- if (phdr->p_filesz && phdr->p_offset < fw->size &&
- phdr->p_offset + phdr->p_filesz <= fw->size) {
- /* Firmware is large enough to be non-split */
- if (phdr->p_offset + phdr->p_filesz > fw->size) {
- dev_err(dev, "file %s segment %d would be truncated\n",
- fw_name, i);
- ret = -EINVAL;
- break;
+ if (phdr->p_filesz) {
+ if (!is_split) {
+ /* Firmware is large enough to be non-split */
+ memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
+ } else {
+ /* Firmware not large enough, load split-out segments */
+ ret = mdt_load_split_segment(ptr, phdrs, i, fw_name, dev);
+ if (ret)
+ break;
}
-
- memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
- } else if (phdr->p_filesz) {
- /* Firmware not large enough, load split-out segments */
- ret = mdt_load_split_segment(ptr, phdrs, i, fw_name, dev);
- if (ret)
- break;
}

if (phdr->p_memsz > phdr->p_filesz)
--
2.25.1


2023-02-13 18:53:47

by Melody Olvera

[permalink] [raw]
Subject: [PATCH 8/9] remoteproc: qcom_q6v5_pas: Add support to attach a DSP

Some chipsets will have DSPs which will have begun running prior
to linux booting, so add support to late attach these DSPs by
adding support for:
- run-time checking of an offline or running DSP via rmb register
- a late attach framework to attach to the running DSP
- a handshake mechanism to ensure full and proper booting via rmb

Signed-off-by: Melody Olvera <[email protected]>
---
drivers/remoteproc/qcom_q6v5_pas.c | 103 ++++++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 1e14ae4d233a..7a1f0f898032 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -224,10 +224,89 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
return ret;
}

+static int adsp_attach(struct rproc *rproc)
+{
+ struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+ int i, ret;
+
+ ret = qcom_q6v5_prepare(&adsp->q6v5);
+ if (ret)
+ return ret;
+
+ ret = adsp_pds_enable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+ if (ret < 0)
+ goto disable_irqs;
+
+ ret = clk_prepare_enable(adsp->xo);
+ if (ret)
+ goto disable_proxy_pds;
+
+ ret = clk_prepare_enable(adsp->aggre2_clk);
+ if (ret)
+ goto disable_xo_clk;
+
+ if (adsp->cx_supply) {
+ ret = regulator_enable(adsp->cx_supply);
+ if (ret)
+ goto disable_aggre2_clk;
+ }
+
+ if (adsp->px_supply) {
+ ret = regulator_enable(adsp->px_supply);
+ if (ret)
+ goto disable_cx_supply;
+ }
+
+ /* if needed, signal Q6 to continute booting */
+ if (adsp->q6v5.rmb_base) {
+ for (i = 0; i < RMB_POLL_MAX_TIMES; i++) {
+ if (readl_relaxed(adsp->q6v5.rmb_base + RMB_BOOT_WAIT_REG)) {
+ writel_relaxed(1, adsp->q6v5.rmb_base + RMB_BOOT_CONT_REG);
+ break;
+ }
+ msleep(20);
+ }
+
+ if (!readl_relaxed(adsp->q6v5.rmb_base + RMB_BOOT_WAIT_REG)) {
+ dev_err(adsp->dev, "Didn't get rmb signal from %s\n", rproc->name);
+ goto disable_px_supply;
+ }
+ }
+
+ ret = qcom_q6v5_wait_for_start(&adsp->q6v5, msecs_to_jiffies(5000));
+ if (ret == -ETIMEDOUT) {
+ dev_err(adsp->dev, "start timed out\n");
+ qcom_scm_pas_shutdown(adsp->pas_id);
+ goto disable_px_supply;
+ }
+
+ return 0;
+
+disable_px_supply:
+ if (adsp->px_supply)
+ regulator_disable(adsp->px_supply);
+disable_cx_supply:
+ if (adsp->cx_supply)
+ regulator_disable(adsp->cx_supply);
+disable_aggre2_clk:
+ clk_disable_unprepare(adsp->aggre2_clk);
+disable_xo_clk:
+ clk_disable_unprepare(adsp->xo);
+disable_proxy_pds:
+ adsp_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
+disable_irqs:
+ qcom_q6v5_unprepare(&adsp->q6v5);
+
+ /* Remove pointer to the loaded firmware, only valid in adsp_load() & adsp_start() */
+ adsp->firmware = NULL;
+
+ return ret;
+}
+
static int adsp_start(struct rproc *rproc)
{
struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
- int ret;
+ int i, ret;

ret = qcom_q6v5_prepare(&adsp->q6v5);
if (ret)
@@ -286,6 +365,22 @@ static int adsp_start(struct rproc *rproc)
goto release_pas_metadata;
}

+ /* if needed, signal Q6 to continute booting */
+ if (adsp->q6v5.rmb_base) {
+ for (i = 0; i < RMB_POLL_MAX_TIMES; i++) {
+ if (readl_relaxed(adsp->q6v5.rmb_base + RMB_BOOT_WAIT_REG)) {
+ writel_relaxed(1, adsp->q6v5.rmb_base + RMB_BOOT_CONT_REG);
+ break;
+ }
+ msleep(20);
+ }
+
+ if (!readl_relaxed(adsp->q6v5.rmb_base + RMB_BOOT_WAIT_REG)) {
+ dev_err(adsp->dev, "Didn't get rmb signal from %s\n", rproc->name);
+ goto release_pas_metadata;
+ }
+ }
+
ret = qcom_q6v5_wait_for_start(&adsp->q6v5, msecs_to_jiffies(5000));
if (ret == -ETIMEDOUT) {
dev_err(adsp->dev, "start timed out\n");
@@ -395,6 +490,7 @@ static unsigned long adsp_panic(struct rproc *rproc)
static const struct rproc_ops adsp_ops = {
.unprepare = adsp_unprepare,
.start = adsp_start,
+ .attach = adsp_attach,
.stop = adsp_stop,
.da_to_va = adsp_da_to_va,
.parse_fw = qcom_register_dump_segments,
@@ -405,6 +501,7 @@ static const struct rproc_ops adsp_ops = {
static const struct rproc_ops adsp_minidump_ops = {
.unprepare = adsp_unprepare,
.start = adsp_start,
+ .attach = adsp_attach,
.stop = adsp_stop,
.da_to_va = adsp_da_to_va,
.load = adsp_load,
@@ -710,6 +807,10 @@ static int adsp_probe(struct platform_device *pdev)
if (ret)
goto detach_proxy_pds;

+ if (adsp->q6v5.rmb_base &&
+ readl_relaxed(adsp->q6v5.rmb_base + RMB_Q6_BOOT_STATUS_REG))
+ rproc->state = RPROC_DETACHED;
+
qcom_add_glink_subdev(rproc, &adsp->glink_subdev, desc->ssr_name);
qcom_add_smd_subdev(rproc, &adsp->smd_subdev);
adsp->sysmon = qcom_add_sysmon_subdev(rproc,
--
2.25.1


2023-02-13 18:53:50

by Melody Olvera

[permalink] [raw]
Subject: [PATCH 9/9] remoteproc: qcom_q6v5_pas: Add QDU1000/QRU1000 mpss compatible & data

Add the compatible and data for the mpss found in the QDU1000 and
QRU1000 SoCs. These platforms require the driver to complete a modem
handshake using the RMB registers provided.

Signed-off-by: Melody Olvera <[email protected]>
---
drivers/remoteproc/qcom_q6v5_pas.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 7a1f0f898032..1ef055b99b66 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -954,6 +954,28 @@ static const struct adsp_data msm8996_adsp_resource = {
.ssctl_id = 0x14,
};

+static const struct adsp_data qdu1000_mpss_resource = {
+ .crash_reason_smem = 421,
+ .firmware_name = "modem.mdt",
+ .dtb_firmware_name = "modem_dtb.mdt",
+ .pas_id = 4,
+ .dtb_pas_id = 0x26,
+ .minidump_id = 3,
+ .has_aggre2_clk = false,
+ .auto_boot = false,
+ .decrypt_shutdown = true,
+ .proxy_pd_names = (char*[]) {
+ "cx",
+ "mss",
+ NULL
+ },
+ .load_state = "modem",
+ .ssr_name = "mpss",
+ .sysmon_name = "modem",
+ .ssctl_id = 0x12,
+ .region_assign_idx = 2,
+};
+
static const struct adsp_data cdsp_resource_init = {
.crash_reason_smem = 601,
.firmware_name = "cdsp.mdt",
@@ -1273,6 +1295,7 @@ static const struct of_device_id adsp_of_match[] = {
{ .compatible = "qcom,qcs404-adsp-pas", .data = &adsp_resource_init },
{ .compatible = "qcom,qcs404-cdsp-pas", .data = &cdsp_resource_init },
{ .compatible = "qcom,qcs404-wcss-pas", .data = &wcss_resource_init },
+ { .compatible = "qcom,qdu1000-mpss-pas", .data = &qdu1000_mpss_resource },
{ .compatible = "qcom,sc7180-mpss-pas", .data = &mpss_resource_init},
{ .compatible = "qcom,sc7280-mpss-pas", .data = &mpss_resource_init},
{ .compatible = "qcom,sc8180x-adsp-pas", .data = &sm8150_adsp_resource},
--
2.25.1


2023-02-14 08:28:45

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 5/9] dt-bindings: remoteproc: mpss: Document QDU1000/QRU1000 mpss devices

On 13/02/2023 19:52, Melody Olvera wrote:
> This documents the compatible for the component used to boot the
> MPSS on the QDU1000 and QRU1000 SoCs.
>
> The QDU1000 and QRU1000 mpss boot process now requires the specification
> of an RMB register space to complete the handshake needed to start or
> attach the mpss.
>
> Signed-off-by: Melody Olvera <[email protected]>
> ---
> .../bindings/remoteproc/qcom,qdu1000-pas.yaml | 127 ++++++++++++++++++
> 1 file changed, 127 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
> new file mode 100644
> index 000000000000..eb6ade984778
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
> @@ -0,0 +1,127 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,qdu1000-pas.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QDU1000 Peripheral Authentication Service
> +
> +maintainers:
> + - Melody Olvera <[email protected]>
> +
> +description:
> + Qualcomm QDU1000 SoC Peripheral Authentication Service loads and boots firmware
> + on the Qualcomm DSP Hexagon cores.
> +
> +properties:
> + compatible:
> + enum:
> + - qcom,qdu1000-mpss-pas

What about other remote processors? The subject prefix suggests it is
only for mpss, but filename is different.

> +
> + reg:
> + maxItems: 2
> +
> + clocks:
> + items:
> + - description: XO clock
> +
> + clock-names:
> + items:
> + - const: xo
> +
> + qcom,qmp:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: Reference to the AOSS side-channel message RAM.
> +
> + smd-edge: false
> +
> + firmware-name:
> + $ref: /schemas/types.yaml#/definitions/string-array
> + items:
> + - description: Firmware name of the Hexagon core
> + - description: Firmware name of the Hexagon Devicetree
> +
> + memory-region:
> + items:
> + - description: Memory region for main Firmware authentication
> + - description: Memory region for Devicetree Firmware authentication
> + - description: DSM Memory region
> +
> + interrupts:
> + minItems: 6
> +
> + interrupt-names:
> + minItems: 6
> +
> + interconnects:
> + minItems: 1

You can drop the property. It's coming from qcom,pas-common.yaml

> +
> + power-domains:
> + items:
> + - description: CX power domain
> + - description: MSS power domain
> +
> + power-domain-names:
> + items:
> + - const: cx
> + - const: mss
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - $ref: /schemas/remoteproc/qcom,pas-common.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/qcom,rpmh.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/mailbox/qcom-ipcc.h>
> +
> + remoteproc@4080000 {
> + compatible = "qcom,qdu1000-mpss-pas";
> + reg = <0x4080000 0x4040>,
> + <0x4180000 0x1000>;
> +
> + clocks = <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "xo";
> +
> + interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
> + <&smp2p_modem_in 0 IRQ_TYPE_EDGE_RISING>,
> + <&smp2p_modem_in 1 IRQ_TYPE_EDGE_RISING>,
> + <&smp2p_modem_in 2 IRQ_TYPE_EDGE_RISING>,
> + <&smp2p_modem_in 3 IRQ_TYPE_EDGE_RISING>
> + <&smp2p_modem_in 7 IRQ_TYPE_EDGE_RISING>;
> + interrupt-names = "wdog", "fatal", "ready", "handover",
> + "stop-ack", "shutdown-ack";
> +
> + memory-region = <&mpss_mem>, <&dtb_mpss_mem>, <&mpss_dsm_mem>;
> +
> + firmware-name = "modem.mdt",
> + "modem_dtb.mdt";
> +
> + power-domains = <&rpmhpd QDU1000_CX>,
> + <&rpmhpd QDU1000_MSS>;
> + power-domain-names = "cx", "mss";
> +
> + interconnects = <&mc_virt MASTER_LLCC &mc_virt SLAVE_EBI1>;
> +
> + qcom,qmp = <&aoss_qmp>;
> +
> + qcom,smem-states = <&smp2p_adsp_out 0>;
> + qcom,smem-state-names = "stop";
> +
> + glink-edge {
> + interrupts-extended = <&ipcc IPCC_CLIENT_MPSS
> + IPCC_MPROC_SIGNAL_GLINK_QMP
> + IRQ_TYPE_EDGE_RISING>;
> + mboxes = <&ipcc IPCC_CLIENT_MPSS IPCC_MPROC_SIGNAL_GLINK_QMP>;
> +
> + label = "modem";
> + qcom,remote-pid = <2>;
> +

Drop blank line

> + };
> + };

Best regards,
Krzysztof


2023-02-14 16:13:24

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 5/9] dt-bindings: remoteproc: mpss: Document QDU1000/QRU1000 mpss devices


On Mon, 13 Feb 2023 10:52:14 -0800, Melody Olvera wrote:
> This documents the compatible for the component used to boot the
> MPSS on the QDU1000 and QRU1000 SoCs.
>
> The QDU1000 and QRU1000 mpss boot process now requires the specification
> of an RMB register space to complete the handshake needed to start or
> attach the mpss.
>
> Signed-off-by: Melody Olvera <[email protected]>
> ---
> .../bindings/remoteproc/qcom,qdu1000-pas.yaml | 127 ++++++++++++++++++
> 1 file changed, 127 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/remoteproc/qcom,pas-common.yaml
Error: Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.example.dts:30.42-43 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:434: Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


2023-02-14 17:36:14

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 6/9] soc: qcom: mdt_loader: Enhance split binary detection

On Mon, Feb 13, 2023 at 10:52:15AM -0800, Melody Olvera wrote:
> From: Gokul Krishna Krishnakumar <[email protected]>
>
> When booting with split binaries, it is possible that the
> mdt loader misdetects if a binary is split and only loads
> one of the segments, so enhance the detection of the split
> binaries to ensure the entirety of the firmware is loaded.

Please describe in detail what it is that is being "misdetected", and
why, so other users can correlate their experience to the changes in the
git log and that reviewers doesn't have to guess what problem is being
corrected.

Thanks,
Bjorn

>
> Signed-off-by: Gokul Krishna Krishnakumar <[email protected]>
> Signed-off-by: Melody Olvera <[email protected]>
> ---
> drivers/soc/qcom/mdt_loader.c | 64 +++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 33dd8c315eb7..3aadce299c02 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
> return true;
> }
>
> +static bool qcom_mdt_bins_are_split(const struct firmware *fw)
> +{
> + const struct elf32_phdr *phdrs;
> + const struct elf32_hdr *ehdr;
> + uint64_t seg_start, seg_end;
> + int i;
> +
> + ehdr = (struct elf32_hdr *)fw->data;
> + phdrs = (struct elf32_phdr *)(ehdr + 1);
> +
> + for (i = 0; i < ehdr->e_phnum; i++) {
> + seg_start = phdrs[i].p_offset;
> + seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
> + if (seg_start > fw->size || seg_end > fw->size)
> + return true;
> + }
> +
> + return false;
> +}
> +
> static ssize_t mdt_load_split_segment(void *ptr, const struct elf32_phdr *phdrs,
> unsigned int segment, const char *fw_name,
> struct device *dev)
> @@ -167,23 +187,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
> /* Copy ELF header */
> memcpy(data, fw->data, ehdr_size);
>
> - if (ehdr_size + hash_size == fw->size) {
> - /* Firmware is split and hash is packed following the ELF header */
> - hash_offset = phdrs[0].p_filesz;
> - memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> - } else if (phdrs[hash_segment].p_offset + hash_size <= fw->size) {
> - /* Hash is in its own segment, but within the loaded file */
> +
> + if (qcom_mdt_bins_are_split(fw)) {
> + ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
> + } else {
> hash_offset = phdrs[hash_segment].p_offset;
> memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> - } else {
> - /* Hash is in its own segment, beyond the loaded file */
> - ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
> - if (ret) {
> - kfree(data);
> - return ERR_PTR(ret);
> - }
> }
> -
> *data_len = ehdr_size + hash_size;
>
> return data;
> @@ -270,6 +280,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> phys_addr_t min_addr = PHYS_ADDR_MAX;
> ssize_t offset;
> bool relocate = false;
> + bool is_split;
> void *ptr;
> int ret = 0;
> int i;
> @@ -277,6 +288,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> if (!fw || !mem_region || !mem_phys || !mem_size)
> return -EINVAL;
>
> + is_split = qcom_mdt_bins_are_split(fw);
> ehdr = (struct elf32_hdr *)fw->data;
> phdrs = (struct elf32_phdr *)(ehdr + 1);
>
> @@ -330,22 +342,16 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>
> ptr = mem_region + offset;
>
> - if (phdr->p_filesz && phdr->p_offset < fw->size &&
> - phdr->p_offset + phdr->p_filesz <= fw->size) {
> - /* Firmware is large enough to be non-split */
> - if (phdr->p_offset + phdr->p_filesz > fw->size) {
> - dev_err(dev, "file %s segment %d would be truncated\n",
> - fw_name, i);
> - ret = -EINVAL;
> - break;
> + if (phdr->p_filesz) {
> + if (!is_split) {
> + /* Firmware is large enough to be non-split */
> + memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> + } else {
> + /* Firmware not large enough, load split-out segments */
> + ret = mdt_load_split_segment(ptr, phdrs, i, fw_name, dev);
> + if (ret)
> + break;
> }
> -
> - memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> - } else if (phdr->p_filesz) {
> - /* Firmware not large enough, load split-out segments */
> - ret = mdt_load_split_segment(ptr, phdrs, i, fw_name, dev);
> - if (ret)
> - break;
> }
>
> if (phdr->p_memsz > phdr->p_filesz)
> --
> 2.25.1
>

2023-02-14 21:23:40

by Melody Olvera

[permalink] [raw]
Subject: Re: [PATCH 5/9] dt-bindings: remoteproc: mpss: Document QDU1000/QRU1000 mpss devices



On 2/14/2023 12:28 AM, Krzysztof Kozlowski wrote:
> On 13/02/2023 19:52, Melody Olvera wrote:
>> This documents the compatible for the component used to boot the
>> MPSS on the QDU1000 and QRU1000 SoCs.
>>
>> The QDU1000 and QRU1000 mpss boot process now requires the specification
>> of an RMB register space to complete the handshake needed to start or
>> attach the mpss.
>>
>> Signed-off-by: Melody Olvera <[email protected]>
>> ---
>> .../bindings/remoteproc/qcom,qdu1000-pas.yaml | 127 ++++++++++++++++++
>> 1 file changed, 127 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>> new file mode 100644
>> index 000000000000..eb6ade984778
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>> @@ -0,0 +1,127 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,qdu1000-pas.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm QDU1000 Peripheral Authentication Service
>> +
>> +maintainers:
>> + - Melody Olvera <[email protected]>
>> +
>> +description:
>> + Qualcomm QDU1000 SoC Peripheral Authentication Service loads and boots firmware
>> + on the Qualcomm DSP Hexagon cores.
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - qcom,qdu1000-mpss-pas
> What about other remote processors? The subject prefix suggests it is
> only for mpss, but filename is different.

Yeah so QDU1000 and QRU1000 only have mpss; there are no other remote processors.
However, it uses the same PAS driver as the other remote processors on other SoCs.
I can rename to Modem Peripheral Authentication Service.

>
>> +
>> + reg:
>> + maxItems: 2
>> +
>> + clocks:
>> + items:
>> + - description: XO clock
>> +
>> + clock-names:
>> + items:
>> + - const: xo
>> +
>> + qcom,qmp:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: Reference to the AOSS side-channel message RAM.
>> +
>> + smd-edge: false
>> +
>> + firmware-name:
>> + $ref: /schemas/types.yaml#/definitions/string-array
>> + items:
>> + - description: Firmware name of the Hexagon core
>> + - description: Firmware name of the Hexagon Devicetree
>> +
>> + memory-region:
>> + items:
>> + - description: Memory region for main Firmware authentication
>> + - description: Memory region for Devicetree Firmware authentication
>> + - description: DSM Memory region
>> +
>> + interrupts:
>> + minItems: 6
>> +
>> + interrupt-names:
>> + minItems: 6
>> +
>> + interconnects:
>> + minItems: 1
> You can drop the property. It's coming from qcom,pas-common.yaml

Got it.

>
>> +
>> + power-domains:
>> + items:
>> + - description: CX power domain
>> + - description: MSS power domain
>> +
>> + power-domain-names:
>> + items:
>> + - const: cx
>> + - const: mss
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +allOf:
>> + - $ref: /schemas/remoteproc/qcom,pas-common.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/clock/qcom,rpmh.h>
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> + #include <dt-bindings/mailbox/qcom-ipcc.h>
>> +
>> + remoteproc@4080000 {
>> + compatible = "qcom,qdu1000-mpss-pas";
>> + reg = <0x4080000 0x4040>,
>> + <0x4180000 0x1000>;
>> +
>> + clocks = <&rpmhcc RPMH_CXO_CLK>;
>> + clock-names = "xo";
>> +
>> + interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
>> + <&smp2p_modem_in 0 IRQ_TYPE_EDGE_RISING>,
>> + <&smp2p_modem_in 1 IRQ_TYPE_EDGE_RISING>,
>> + <&smp2p_modem_in 2 IRQ_TYPE_EDGE_RISING>,
>> + <&smp2p_modem_in 3 IRQ_TYPE_EDGE_RISING>
>> + <&smp2p_modem_in 7 IRQ_TYPE_EDGE_RISING>;
>> + interrupt-names = "wdog", "fatal", "ready", "handover",
>> + "stop-ack", "shutdown-ack";
>> +
>> + memory-region = <&mpss_mem>, <&dtb_mpss_mem>, <&mpss_dsm_mem>;
>> +
>> + firmware-name = "modem.mdt",
>> + "modem_dtb.mdt";
>> +
>> + power-domains = <&rpmhpd QDU1000_CX>,
>> + <&rpmhpd QDU1000_MSS>;
>> + power-domain-names = "cx", "mss";
>> +
>> + interconnects = <&mc_virt MASTER_LLCC &mc_virt SLAVE_EBI1>;
>> +
>> + qcom,qmp = <&aoss_qmp>;
>> +
>> + qcom,smem-states = <&smp2p_adsp_out 0>;
>> + qcom,smem-state-names = "stop";
>> +
>> + glink-edge {
>> + interrupts-extended = <&ipcc IPCC_CLIENT_MPSS
>> + IPCC_MPROC_SIGNAL_GLINK_QMP
>> + IRQ_TYPE_EDGE_RISING>;
>> + mboxes = <&ipcc IPCC_CLIENT_MPSS IPCC_MPROC_SIGNAL_GLINK_QMP>;
>> +
>> + label = "modem";
>> + qcom,remote-pid = <2>;
>> +
> Drop blank line

Ok.

Thanks,
Melody
>
>> + };
>> + };
> Best regards,
> Krzysztof
>


2023-02-15 20:20:36

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 5/9] dt-bindings: remoteproc: mpss: Document QDU1000/QRU1000 mpss devices

On 14/02/2023 22:23, Melody Olvera wrote:
>
>
> On 2/14/2023 12:28 AM, Krzysztof Kozlowski wrote:
>> On 13/02/2023 19:52, Melody Olvera wrote:
>>> This documents the compatible for the component used to boot the
>>> MPSS on the QDU1000 and QRU1000 SoCs.
>>>
>>> The QDU1000 and QRU1000 mpss boot process now requires the specification
>>> of an RMB register space to complete the handshake needed to start or
>>> attach the mpss.
>>>
>>> Signed-off-by: Melody Olvera <[email protected]>
>>> ---
>>> .../bindings/remoteproc/qcom,qdu1000-pas.yaml | 127 ++++++++++++++++++
>>> 1 file changed, 127 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>>> new file mode 100644
>>> index 000000000000..eb6ade984778
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,qdu1000-pas.yaml
>>> @@ -0,0 +1,127 @@
>>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/remoteproc/qcom,qdu1000-pas.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm QDU1000 Peripheral Authentication Service
>>> +
>>> +maintainers:
>>> + - Melody Olvera <[email protected]>
>>> +
>>> +description:
>>> + Qualcomm QDU1000 SoC Peripheral Authentication Service loads and boots firmware
>>> + on the Qualcomm DSP Hexagon cores.
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - qcom,qdu1000-mpss-pas
>> What about other remote processors? The subject prefix suggests it is
>> only for mpss, but filename is different.
>
> Yeah so QDU1000 and QRU1000 only have mpss; there are no other remote processors.
> However, it uses the same PAS driver as the other remote processors on other SoCs.
> I can rename to Modem Peripheral Authentication Service.

Yes, please rename the title. Also please rename the file (and $id) to
be based on compatible:
qcom,qdu1000-mpss-pas.yaml



Best regards,
Krzysztof