| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <79e99777-3d1e-4451-a932-9d2a68192ee5@codeaurora.org>
Date: Mon, 31 Jul 2017 16:21:47 +0530
From: Sricharan R <sricharan@...eaurora.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: ohad@...ery.com, robh+dt@...nel.org, mark.rutland@....com,
andy.gross@...aro.org, david.brown@...aro.org,
linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-soc@...r.kernel.org
Subject: Re: [PATCH 2/3] drivers: remoteproc: Add Hexagon Q6 - WCSS integrated
core driver
Hi Bjorn,
On 7/27/2017 12:38 AM, Bjorn Andersson wrote:
> On Thu 29 Jun 07:17 PDT 2017, Sricharan R wrote:
>
>> IPQ8074 has an integrated Hexagon dsp core Q6v5 and a wireless lan
>> (Lithium) IP. This patch adds the remoteproc driver to reset, load
>> and boot Q6 firmware.
>>
>
> There is a fair amount of code in this driver that seems to be
> equivalent to Avaneesh q6v5 patches for MSM8996.
>
> The differences coming from the MBA + MPSS vs single-image we have to
> live with, but can we do something about the Q6 programming sequences?
> E.g. extract them to a common file?
>
hmm, initially was trying to do that, but Q6 programming sequence was
not that much common. So added this as a new driver. But let me try
once more.
>> Signed-off-by: Sricharan R <sricharan@...eaurora.org>
>> ---
>> drivers/remoteproc/Kconfig | 13 +
>> drivers/remoteproc/Makefile | 1 +
>> drivers/remoteproc/q6v5-wcss.c | 784 +++++++++++++++++++++++++++++++++++++++++
>
> Please keep the qcom_* naming scheme.
>
ok
> [..]
>> diff --git a/drivers/remoteproc/q6v5-wcss.c b/drivers/remoteproc/q6v5-wcss.c
> [..]
>> +#include <linux/elf.h>
>
> Doesn't look like you need this anymore.
>
ok
> [..]
>> +#include <linux/qcom_scm.h>
>
> You don't need this.
>
right, will remove
> [..]
>> +
>> +#define WCSS_CRASH_REASON_SMEM 421
>> +#define WCNSS_PAS_ID 6
>> +#define STOP_ACK_TIMEOUT_MS 10000
>> +
>> +#define QDSP6SS_RST_EVB 0x10
>> +#define QDSP6SS_RESET 0x14
>> +#define QDSP6SS_DBG_CFG 0x18
>> +#define QDSP6SS_XO_CBCR 0x38
>> +#define QDSP6SS_MEM_PWR_CTL 0xb0
>> +#define QDSP6SS_BHS_STATUS 0x78
>> +#define TCSR_GLOBAL_CFG0 0x0
>> +#define TCSR_GLOBAL_CFG1 0x4
>> +
>> +#define QDSP6SS_GFMUX_CTL 0x20
>> +#define QDSP6SS_PWR_CTL 0x30
>> +#define TCSR_HALTREQ 0x0
>> +#define TCSR_HALTACK 0x4
>> +#define TCSR_Q6_HALTREQ 0x0
>> +#define TCSR_Q6_HALTACK 0x4
>> +#define SSCAON_CONFIG 0x8
>> +#define SSCAON_STATUS 0xc
>> +#define HALTACK BIT(0)
>> +#define BHS_EN_REST_ACK BIT(0)
>
> It would be nice to have the values of these indented, to make things a
> little bit easier to read.
>
ok
> [..]
>> +static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
>> + const struct firmware *fw,
>> + int *tablesz)
>
> You can omit this function, there's a dummy in qcom_common.[ch] with the
> same name for the same purpose.
>
ok
>> +{
>> + static struct resource_table table = { .ver = 1, };
>> +
>> + *tablesz = sizeof(table);
>> + return &table;
>> +}
>> +
>> +static int q6v5_init_clocks(struct device *dev, struct q6v5 *qproc)
>> +{
>> + int i;
>> + const char *cname;
>> + struct property *prop;
>> +
>> + qproc->clk_cnt = of_property_count_strings(dev->of_node,
>> + "clock-names");
>> +
>> + if (!qproc->clk_cnt)
>> + return 0;
>
> I strongly prefer that you explicitly list the clocks expected here,
> rather than trusting DT.
>
ok.
> And please transition this to the newly added clk-bulk interface (see
> clk_bulk_get() et al).
>
ok.
>> +
>> + qproc->clks = devm_kzalloc(dev, sizeof(*qproc->clks) * qproc->clk_cnt,
>> + GFP_KERNEL);
>> +
>> + of_property_for_each_string(dev->of_node, "clock-names", prop, cname) {
>> + struct clk *c = devm_clk_get(dev, cname);
>> +
>> + if (IS_ERR_OR_NULL(c)) {
>> + if (PTR_ERR(c) != -EPROBE_DEFER)
>> + dev_err(dev, "Failed to get %s clock\n", cname);
>> +
>> + return PTR_ERR(c);
>> + }
>> +
>> + qproc->clks[i++] = c;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int q6v5_clk_enable(struct q6v5 *qproc)
>> +{
>> + int rc;
>> + int i;
>> +
>> + for (i = 0; i < qproc->clk_cnt; i++) {
>> + rc = clk_prepare_enable(qproc->clks[i]);
>> + if (rc)
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + for (i--; i >= 0; i--)
>> + clk_disable_unprepare(qproc->clks[i]);
>> +
>> + return rc;
>> +}
>
> As of v4.13-rc1 you can call clk_bulk_prepare_enable() instead of this
> function.
>
ok.
>> +
>> +static int wcss_powerdown(struct q6v5 *qproc)
>> +{
>> + unsigned int nretry = 0;
>> + unsigned int val = 0;
>> + int ret;
>> +
>> + /* Assert WCSS/Q6 HALTREQ - 1 */
>
> I presume the numbers at the end of these comments corresponds to the
> steps in your programming manual, if so it's okay to keep them. But
> please make move them to the front, like /* N: comment */
>
right, it corresponds to the manual. Will change as suggested.
>> + nretry = 0;
>> +
>> + ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
>> + 1, 1);
>
> Is there a reason to do read-modify-write on this register? I use
> regmap_write() in the other driver.
>
Just did a ditto of what was mentioned in the manual. May be a direct write
would also suffice. Will check and update if it works.
>> + if (ret)
>> + return ret;
>> +
>> + while (1) {
>> + regmap_read(qproc->tcsr, qproc->halt_wcss + TCSR_HALTACK,
>> + &val);
>> + if (val & HALTACK)
>> + break;
>> + mdelay(1);
>> + nretry++;
>> + if (nretry >= 10) {
>> + pr_warn("can't get TCSR haltACK\n");
>> + break;
>> + }
>> + }
>
> Would it be possible to move q6v5proc_halt_axi_port() to qcom_common.c
> (or a tcsr driver?) and use that instead?
>
ok, qcom_common.c, this can be used in both qcom_q6v5_pil.c as well.
>> +
>> + /* Check HALTACK */
>
> I presume this comment does not relate to the code that follows it?
>
ok, will remove.
>> + /* Set MPM_SSCAON_CONFIG 13 - 2 */
>
> Shouldn't this be "Disable MPM_WCSSAON_CONFIG 13"?
>
Right, will update.
>> + val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> + val |= BIT(13);
>> + writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> + /* Set MPM_SSCAON_CONFIG 15 - 3 */
>> + val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> + val |= BIT(15);
>> + val &= ~(BIT(16));
>> + val &= ~(BIT(17));
>> + val &= ~(BIT(18));
>
> Skip all the extra parenthesis.
>
ok.
>> + writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> + /* Set MPM_SSCAON_CONFIG 1 - 4 */
>> + val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> + val |= BIT(1);
>> + writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> + /* wait for SSCAON_STATUS to be 0x400 - 5 */
>> + nretry = 0;
>> + while (1) {
>> + val = readl(qproc->mpm_base + SSCAON_STATUS);
>> + /* ignore bits 16 to 31 */
>> + val &= 0xffff;
>> + if (val == BIT(10))
>> + break;
>> + nretry++;
>> + mdelay(1);
>> + if (nretry == 10) {
>> + pr_warn("can't get SSCAON_STATUS\n");
>> + break;
>> + }
>> + }
>
> Please replace this loop with:
> ret = readl_poll_timeout(qproc->mpm_base + SSCAON_STATUS, val,
> val & 0xffff == 0x400, 1000, 10000);
>
sure, thanks.
>> +
>> + /* Enable Q6/WCSS BLOCK ARES - 6 */
>> + reset_control_assert(qproc->wcss_aon_reset);
>> +
>> + /* Enable MPM_WCSSAON_CONFIG 13 - 7 */
>> + val = readl(qproc->mpm_base + SSCAON_CONFIG);
>> + val &= (~(BIT(13)));
>
> Skip all the extra parenthesis.
>
ok.
>> + writel(val, qproc->mpm_base + SSCAON_CONFIG);
>> +
>> + /* Enable A2AB/ACMT/ECHAB ARES - 8 */
>> + /* De-assert WCSS/Q6 HALTREQ - 8 */
>> + reset_control_assert(qproc->wcss_reset);
>> +
>> + ret = regmap_update_bits(qproc->tcsr, qproc->halt_wcss + TCSR_HALTREQ,
>> + 1, 0);
>> +
>> + return ret;
>> +}
>> +
>> +static int q6_powerdown(struct q6v5 *qproc)
>> +{
>> + int i = 0, ret;
>> + unsigned int nretry = 0;
>> + unsigned int val = 0;
>> +
>> + /* Halt Q6 bus interface - 9*/
>> + ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
>> + 1, 1);
>> + if (ret)
>> + return ret;
>> +
>> + nretry = 0;
>> + while (1) {
>> + regmap_read(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTACK,
>> + &val);
>> + if (val & HALTACK)
>> + break;
>> + mdelay(1);
>> + nretry++;
>> + if (nretry >= 10) {
>> + pr_err("can't get TCSR Q6 haltACK\n");
>> + break;
>> + }
>> + }
>
> Again, can we utilize q6v5proc_halt_axi_port()? (Or replace the tcsr
> syscon with a driver)
>
Ya, the halt_axi_port across both of these Q6 drivers can be put in
qcom_common.c.
>> +
>> + /* Disable Q6 Core clock - 10 */
>> + val = readl(qproc->q6_base + QDSP6SS_GFMUX_CTL);
>> + val &= (~(BIT(1)));
>
> Parenthesis.
>
ok.
>> + writel(val, qproc->q6_base + QDSP6SS_GFMUX_CTL);
>> +
>> + /* Clamp I/O - 11 */
>> + val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> + val |= BIT(20);
>> + writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* Clamp WL - 12 */
>> + val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> + val |= BIT(21);
>> + writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* Clear Erase standby - 13 */
>> + val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> + val &= (~(BIT(18)));
>> + writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* Clear Sleep RTN - 14 */
>> + val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> + val &= (~(BIT(19)));
>> + writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* turn off QDSP6 memory foot/head switch one bank at a time - 15*/
>> + for (i = 0; i < 20; i++) {
>> + val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> + val &= (~(BIT(i)));
>
> Parenthesis.
>
ok.
>> + writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> + mdelay(1);
>> + }
>> +
>> + /* Assert QMC memory RTN - 16 */
>> + val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> + val |= BIT(22);
>> + writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* Turn off BHS - 17 */
>> + val = readl(qproc->q6_base + QDSP6SS_PWR_CTL);
>> + val &= (~(BIT(24)));
>> + writel(val, qproc->q6_base + QDSP6SS_PWR_CTL);
>> + udelay(1);
>> + /* Wait till BHS Reset is done */
>> + nretry = 0;
>> + while (1) {
>> + val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
>> + if (!(val & BHS_EN_REST_ACK))
>> + break;
>> + mdelay(1);
>> + nretry++;
>> + if (nretry >= 10) {
>> + pr_err("BHS_STATUS not OFF\n");
>> + break;
>> + }
>> + }
>
> readl_poll_timeout()
>
ok.
>> +
>> + /* HALT CLEAR - 18 */
>> + ret = regmap_update_bits(qproc->tcsr, qproc->halt_q6 + TCSR_Q6_HALTREQ,
>> + 1, 0);
>> + if (ret)
>> + return ret;
>> +
>> + /* Enable Q6 Block reset - 19 */
>> + reset_control_assert(qproc->wcss_q6_reset);
>> +
>> + return 0;
>> +}
>> +
>> +static int q6_rproc_stop(struct rproc *rproc)
>> +{
>> + struct q6v5 *qproc = rproc->priv;
>> + int ret = 0;
>> +
>> + qproc->running = false;
>> +
>> + /* WCSS powerdown */
>> + qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit),
>> + BIT(qproc->stop_bit));
>> +
>> + ret = wait_for_completion_timeout(&qproc->stop_done,
>> + msecs_to_jiffies(5000));
>> + if (ret == 0) {
>> + dev_err(qproc->dev, "timed out on wait\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0);
>> +
>> + ret = wcss_powerdown(qproc);
>> + if (ret)
>> + return ret;
>> +
>> + /* Q6 Power down */
>> + ret = q6_powerdown(qproc);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int q6_rproc_start(struct rproc *rproc)
>> +{
>> + struct q6v5 *qproc = rproc->priv;
>> + int temp = 19;
>> + unsigned long val = 0;
>> + unsigned int nretry = 0;
>> + int ret = 0;
>> +
>> + ret = q6v5_clk_enable(qproc);
>> + if (ret) {
>> + dev_err(qproc->dev, "failed to enable clocks\n");
>> + return ret;
>> + }
>> +
>> + /* Release Q6 and WCSS reset */
>> + reset_control_deassert(qproc->wcss_reset);
>> + reset_control_deassert(qproc->wcss_q6_reset);
>> +
>> + /* Lithium configuration - clock gating and bus arbitration */
>
> Should this go in a tcsr driver?
>
Not sure i understand this. So you mean we should have a driver that
wrappers the access to tcsr registers. So that means that driver
populates the syscon_to_regmap and passes back the regmap handle.
Then the client driver like Q6 uses tcsr_ apis on top of that ?
>> + ret = regmap_update_bits(qproc->tcsr,
>> + qproc->halt_gbl + TCSR_GLOBAL_CFG0,
>> + 0x1F, 0x14);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_update_bits(qproc->tcsr,
>> + qproc->halt_gbl + TCSR_GLOBAL_CFG1,
>> + 1, 0);
>> + if (ret)
>> + return ret;
>> +
>> + /* Write bootaddr to EVB so that Q6WCSS will jump there after reset */
>> + writel(rproc->bootaddr >> 4, qproc->q6_base + QDSP6SS_RST_EVB);
>> + /* Turn on XO clock. It is required for BHS and memory operation */
>> + writel(0x1, qproc->q6_base + QDSP6SS_XO_CBCR);
>> + /* Turn on BHS */
>> + writel(0x1700000, qproc->q6_base + QDSP6SS_PWR_CTL);
>
> Avaneesh provided defines for most of these magic numbers, please follow
> that.
>
ok.
>> + udelay(1);
>> +
>> + /* Wait till BHS Reset is done */
>> + while (1) {
>> + val = readl(qproc->q6_base + QDSP6SS_BHS_STATUS);
>> + if (val & BHS_EN_REST_ACK)
>> + break;
>> + mdelay(1);
>> + nretry++;
>> + if (nretry >= 10) {
>> + pr_err("BHS_STATUS not ON\n");
>> + break;
>> + }
>> + }
>
> Use readl_poll_timeout()
>
ok.
>> +
>> + /* Put LDO in bypass mode */
>> + writel(0x3700000, qproc->q6_base + QDSP6SS_PWR_CTL);
>> + /* De-assert QDSP6 complier memory clamp */
>> + writel(0x3300000, qproc->q6_base + QDSP6SS_PWR_CTL);
>> + /* De-assert memory peripheral sleep and L2 memory standby */
>> + writel(0x33c0000, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* turn on QDSP6 memory foot/head switch one bank at a time */
>> + while (temp >= 0) {
>
> Please use a for loop, and replace "temp" with "i".
>
ok.
> This is identical to Avaneesh patch, so let's make sure the code is
> identical as well.
>
ok.
>
>> + val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> + val = val | 1 << temp;
>
> val |= BIT(temp);
>
>> + writel(val, qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>> + val = readl(qproc->q6_base + QDSP6SS_MEM_PWR_CTL);
>
> Please include a comment on the read back here.
>
ok.
>> + mdelay(10);
>> + temp -= 1;
>> + }
>> + /* Remove the QDSP6 core memory word line clamp */
>> + writel(0x31FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
>> + /* Remove QDSP6 I/O clamp */
>> + writel(0x30FFFFF, qproc->q6_base + QDSP6SS_PWR_CTL);
>> +
>> + /* Bring Q6 out of reset and stop the core */
>> + writel(0x5, qproc->q6_base + QDSP6SS_RESET);
>> +
>> + /* Retain debugger state during next QDSP6 reset */
>> + writel(0x0, qproc->q6_base + QDSP6SS_DBG_CFG);
>> + /* Turn on the QDSP6 core clock */
>> + writel(0x102, qproc->q6_base + QDSP6SS_GFMUX_CTL);
>> + /* Enable the core to run */
>> + writel(0x4, qproc->q6_base + QDSP6SS_RESET);
>> +
>> + ret = wait_for_completion_timeout(&qproc->start_done,
>> + msecs_to_jiffies(5000));
>> + if (ret == 0) {
>> + dev_err(qproc->dev, "start timed out\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + qproc->running = true;
>> +
>> + return 0;
>> +}
>> +
>> +static struct rproc_ops q6v5_rproc_ops = {
>> + .start = q6_rproc_start,
>> + .stop = q6_rproc_stop,
>> +};
>> +
>> +static struct rproc_fw_ops q6_fw_ops;
>> +
>> +static int q6v5_request_irq(struct q6v5 *qproc,
>> + struct platform_device *pdev,
>> + const char *name,
>> + irq_handler_t thread_fn)
>> +{
>> + int ret;
>> +
>> + ret = platform_get_irq_byname(pdev, name);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "no %s IRQ defined\n", name);
>> + return ret;
>> + }
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, ret,
>> + NULL, thread_fn,
>> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> + "q6v5", qproc);
>> + if (ret)
>> + dev_err(&pdev->dev, "request %s IRQ failed\n", name);
>> +
>> + return ret;
>> +}
>> +
>> +static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev)
>> +{
>> + struct q6v5 *qproc = dev;
>> + char *msg;
>> + size_t len;
>> +
>> + if (!qproc->running)
>> + return IRQ_HANDLED;
>> +
>> + msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
>> + if (!IS_ERR(msg) && len > 0 && msg[0])
>> + dev_err(qproc->dev, "Fatal error from wcss: %s\n", msg);
>> + else
>> + dev_err(qproc->dev, "Fatal error received no message!\n");
>> +
>> + rproc_report_crash(qproc->rproc, RPROC_FATAL_ERROR);
>> +
>> + if (!IS_ERR(msg))
>> + msg[0] = '\0';
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
>> +{
>> + struct q6v5 *qproc = dev;
>> +
>> + complete(&qproc->start_done);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_stop_ack_interrupt(int irq, void *dev)
>> +{
>> + struct q6v5 *qproc = dev;
>> +
>> + complete(&qproc->stop_done);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
>> +{
>> + struct q6v5 *qproc = dev;
>> + char *msg;
>> + size_t len;
>> +
>> + if (!qproc->running) {
>> + complete(&qproc->stop_done);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, WCSS_CRASH_REASON_SMEM, &len);
>> + if (!IS_ERR(msg) && len > 0 && msg[0])
>> + dev_err(qproc->dev, "Watchdog bite from wcss %s\n", msg);
>> + else
>> + dev_err(qproc->dev, "Watchdog bit received no message!\n");
>> +
>> + rproc_report_crash(qproc->rproc, RPROC_WATCHDOG);
>> +
>> + if (!IS_ERR(msg))
>> + msg[0] = '\0';
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>> +{
>> + struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> +
>> + return qcom_mdt_load(qproc->dev, fw, rproc->firmware, WCNSS_PAS_ID,
>> + qproc->mem_region, qproc->mem_phys,
>> + qproc->mem_size, false);
>> +}
>> +
>> +static int q6_alloc_memory_region(struct q6v5 *qproc)
>> +{
>> + struct device_node *node;
>> + struct resource r;
>> + int ret;
>> +
>> + node = of_parse_phandle(qproc->dev->of_node, "memory-region", 0);
>> + if (!node) {
>> + dev_err(qproc->dev, "no memory-region specified\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = of_address_to_resource(node, 0, &r);
>> + if (ret)
>> + return ret;
>> +
>> + qproc->mem_phys = r.start;
>> + qproc->mem_size = resource_size(&r);
>> + qproc->mem_region = devm_ioremap_wc(qproc->dev, qproc->mem_phys,
>> + qproc->mem_size);
>> + if (!qproc->mem_region) {
>> + dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
>> + &r.start, qproc->mem_size);
>> + return -EBUSY;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
>> +{
>> + struct of_phandle_args args;
>> + struct resource *res;
>> + int ret;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> + "mpm");
>> + if (IS_ERR_OR_NULL(res))
>> + return -ENODEV;
>> +
>> + qproc->mpm_base = ioremap(res->start, resource_size(res));
>> + if (IS_ERR_OR_NULL(qproc->mpm_base))
>> + return PTR_ERR(qproc->mpm_base);
>
> Is this the same MPM block that is used to configure sleep related
> things later? If so I think we shouldn't map it directly here, but
> introduce a separate driver for it.
>
Yeah, thats the one. But in this context it is used to configure
some magic register. The downstream's irqchip type of functionality
is not used here. Infact there may not be a usecase at all for this.
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> + "q6");
>> + if (IS_ERR_OR_NULL(res)) {
>> + ret = -ENODEV;
>> + goto free_mpm;
>> + }
>> +
>> + qproc->q6_base = ioremap(res->start, resource_size(res));
>
> Except for the error path of this function these memory regions are
> never unmapped. Please use devm_ioremap_wc() instead.
>
ok.
>> + if (IS_ERR_OR_NULL(qproc->q6_base)) {
>> + ret = PTR_ERR(qproc->q6_base);
>> + goto free_mpm;
>> + }
>> +
>> + ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>> + "qcom,halt-regs", 3,
>> + 0, &args);
>> + if (ret < 0)
>> + goto free_q6;
>> +
>> + qproc->tcsr = syscon_node_to_regmap(args.np);
>> + of_node_put(args.np);
>> + if (IS_ERR_OR_NULL(qproc->tcsr)) {
>> + ret = PTR_ERR(qproc->tcsr);
>> + goto free_q6;
>> + }
>> +
>> + qproc->halt_gbl = args.args[0];
>> + qproc->halt_q6 = args.args[1];
>> + qproc->halt_wcss = args.args[2];
>> +
>> + return 0;
>> +
>> +free_q6:
>> + iounmap(qproc->q6_base);
>> +
>> +free_mpm:
>> + iounmap(qproc->mpm_base);
>> +
>> + return ret;
>> +}
>> +
>> +static int q6_rproc_probe(struct platform_device *pdev)
>> +{
>> + struct q6v5 *qproc;
>> + struct rproc *rproc;
>> + int ret;
>> + struct qcom_smem_state *state;
>> + unsigned int stop_bit;
>> + const char *firmware_name = of_device_get_match_data(&pdev->dev);
>> +
>> + state = qcom_smem_state_get(&pdev->dev, "stop",
>> + &stop_bit);
>> + if (IS_ERR(state))
>> + /* Wait till SMP2P is registered and up */
>> + return -EPROBE_DEFER;
>> +
>> + rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_rproc_ops,
>> + firmware_name,
>> + sizeof(*qproc));
>> + if (unlikely(!rproc))
>> + return -ENOMEM;
>> +
>> + qproc = rproc->priv;
>> + qproc->dev = &pdev->dev;
>> + qproc->rproc = rproc;
>> + rproc->has_iommu = false;
>> +
>> + q6_fw_ops = *rproc->fw_ops;
>> + q6_fw_ops.find_rsc_table = q6v5_find_rsc_table;
>> + q6_fw_ops.load = q6v5_load;
>
> I would prefer that you do define a static const object with these, like
> in the other qcom drivers.
>
ok.
> But in order to do that you would need to export
> rproc_elf_get_boot_addr(), which is in line with another item on my todo
> list, so please do this.
>
> [..]
hmm, in this case, boot_addr is not programmed. So would we still require the
rproc_elf_get_boot_addr ?
Regards,
Sricharan
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
Powered by blists - more mailing lists