Re: [PATCH V2 10/13] remoteproc: qcom: Add Hexagon based multipd rproc driver

[Manikanta Mylavarapu <quic_mmanikan@quicinc.com>]

On 6/27/2023 6:09 PM, Manikanta Mylavarapu wrote:
> 
> 
> On 6/24/2023 12:58 PM, Krzysztof Kozlowski wrote:
>> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
>>> It adds support to bring up remoteproc's on multipd model.
>>> Pd means protection domain. It's similar to process in Linux.
>>> Here QDSP6 processor runs each wifi radio functionality on a
>>> separate process. One process can't access other process
>>> resources, so this is termed as PD i.e protection domain.
>>>
>>> Here we have two pd's called root and user pd. We can correlate
>>> Root pd as root and user pd as user in linux. Root pd has more
>>> privileges than user pd. Root will provide services to user pd.
>>>
>>>  From remoteproc driver perspective, root pd corresponds to QDSP6
>>> processor bring up and user pd corresponds to Wifi radio (WCSS)
>>> bring up.
>>>
>>> Here WCSS(user) PD is dependent on Q6(root) PD, so first
>>> q6 pd should be up before wcss pd. After wcss pd goes down,
>>> q6 pd should be turned off.
>>>
>>> IPQ5018, IPQ9574 supports multipd remoteproc driver.
>>>
>>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>> ---
>>> Changes in V2:
>>>     - Common functionalities moved to seperate patches
>>>     - qcom_get_pd_asid() moved to mpd driver
>>>     - Last DMA block alone memset to zero
>>>     - Added diagram to show how userpd data is organized and sent to
>>>       trustzone
>>>     - Rewritten commit message since most of the content available
>>>       in cover page
>>>     - Removed 'remote_id' becuase it's not required for bring up.
>>>
>>>   drivers/remoteproc/Kconfig          |  20 +
>>>   drivers/remoteproc/Makefile         |   1 +
>>>   drivers/remoteproc/qcom_common.h    |   9 +
>>>   drivers/remoteproc/qcom_q6v5_mpd.c  | 677 ++++++++++++++++++++++++++++
>>>   drivers/soc/qcom/mdt_loader.c       | 332 ++++++++++++++
>>>   include/linux/soc/qcom/mdt_loader.h |  19 +
>>>   6 files changed, 1058 insertions(+)
>>>   create mode 100644 drivers/remoteproc/qcom_q6v5_mpd.c
>>>
>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>> index a850e9f486dd..44af5c36f67e 100644
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -234,6 +234,26 @@ config QCOM_Q6V5_PAS
>>>         CDSP (Compute DSP), MPSS (Modem Peripheral SubSystem), and
>>>         SLPI (Sensor Low Power Island).
>>>
>>> +config QCOM_Q6V5_MPD
>>> +    tristate "Qualcomm Hexagon based MPD model Peripheral Image Loader"
>>> +    depends on OF && ARCH_QCOM
>>> +    depends on QCOM_SMEM
>>> +    depends on RPMSG_QCOM_SMD || RPMSG_QCOM_SMD=n
>>> +    depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
>>> +    depends on QCOM_SYSMON || QCOM_SYSMON=n
>>> +    depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
>>> +    depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
>>> +    select MFD_SYSCON
>>
>> Do you really need this?
>>
	It's not required. I will remove. Thanks for catching.

>>> +    select QCOM_MDT_LOADER
>>> +    select QCOM_PIL_INFO
>>> +    select QCOM_Q6V5_COMMON
>>> +    select QCOM_RPROC_COMMON
>>> +    select QCOM_SCM
>>> +    help
>>> +      Say y here to support the Qualcomm Secure Peripheral Image Loader
>>> +      for the Hexagon based MultiPD model remote processors on e.g. 
>>> IPQ5018.
>>> +      This is trustZone wireless subsystem.
>>> +
>>
>> ...
>>
	I didn't understand. Can you please elaborate your comment?

>>> +    int (*powerup_scm)(u32 peripheral);
>>> +    int (*powerdown_scm)(u32 peripheral);
>>> +};
>>> +
>>> +/**
>>> + * qcom_get_pd_asid() - get the pd asid number from DT node
>>
>>  From node name? NAK. It does not work like that. Node names can change
>> and you did not define this number as part of ABI.
>>
>> Probably you wanted unit address.
>>

	Yeah i got your point. Each of the WCSS PD's are internally
	represented in Q6 with their corresponding "spawn" bit numbers.
	I will use same and remove the "PD-" hardcodings.

	Is this fine ?

>>> + * @node:    device tree node
>>> + *
>>> + * Returns asid if node name has 'pd' string
>>> + */
>>> +s8 qcom_get_pd_asid(struct device_node *node)
>>> +{
>>> +    char *str;
>>> +    s8 pd_asid;
>>> +
>>> +    if (!node)
>>> +        return -EINVAL;
>>> +
>>> +    str = strstr(node->name, "pd");
>>> +    if (!str)
>>> +        return 0;
>>> +
>>> +    str += strlen("pd") + 1;
>>> +    return kstrtos8(str, 10, &pd_asid) ? -EINVAL : pd_asid;
>>> +}
>>> +EXPORT_SYMBOL(qcom_get_pd_asid);
>>> +
>>
>> ...
>>
	
	I didn't understand. Can you please elaborate your comment?

>>> +
>>> +static int q6_wcss_spawn_pd(struct rproc *rproc)
>>> +{
>>> +    int ret;
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +
>>> +    ret = qcom_q6v5_request_spawn(&wcss->q6);
>>> +    if (ret == -ETIMEDOUT) {
>>> +        pr_err("%s spawn timedout\n", rproc->name);
>>
>> dev_err
>>

	Sure, I will change to dev_err.

>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = qcom_q6v5_wait_for_start(&wcss->q6, msecs_to_jiffies(10000));
>>> +    if (ret == -ETIMEDOUT) {
>>> +        pr_err("%s start timedout\n", rproc->name);
>>
>> dev_err
>>

	Sure, I will change to dev_err.

>>> +        wcss->q6.running = false;
>>> +        return ret;
>>> +    }
>>> +    wcss->q6.running = true;
>>> +    return ret;
>>> +}
>>> +
>>> +static int wcss_ahb_pcie_pd_start(struct rproc *rproc)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    if (!desc->reset_seq)
>>> +        return 0;
>>> +
>>> +    if (desc->powerup_scm) {
>>> +        ret = desc->powerup_scm(desc->pasid);
>>> +        if (ret) {
>>> +            dev_err(wcss->dev, "failed to power up pd\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    ret = q6_wcss_spawn_pd(rproc);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    wcss->state = WCSS_NORMAL;
>>> +    return ret;
>>> +}
>>> +
>>> +static int q6_wcss_stop(struct rproc *rproc)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    ret = qcom_scm_pas_shutdown(desc->pasid);
>>> +    if (ret) {
>>> +        dev_err(wcss->dev, "not able to shutdown\n");
>>> +        return ret;
>>> +    }
>>> +    qcom_q6v5_unprepare(&wcss->q6);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int wcss_ahb_pcie_pd_stop(struct rproc *rproc)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    struct rproc *rpd_rproc = dev_get_drvdata(wcss->dev->parent);
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    if (!desc->reset_seq)
>>> +        goto shut_down_rpd;
>>> +
>>> +    if (rproc->state != RPROC_CRASHED && wcss->q6.stop_bit) {
>>> +        ret = qcom_q6v5_request_stop(&wcss->q6, NULL);
>>> +        if (ret) {
>>> +            dev_err(&rproc->dev, "pd not stopped\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    if (desc->powerdown_scm) {
>>> +        ret = desc->powerdown_scm(desc->pasid);
>>> +        if (ret) {
>>> +            dev_err(wcss->dev, "failed to power down pd\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +shut_down_rpd:
>>> +    rproc_shutdown(rpd_rproc);
>>> +
>>> +    wcss->state = WCSS_SHUTDOWN;
>>> +    return 0;
>>> +}
>>> +
>>> +static void *q6_wcss_da_to_va(struct rproc *rproc, u64 da, size_t len,
>>> +                  bool *is_iomem)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    int offset;
>>> +
>>> +    offset = da - wcss->mem_reloc;
>>> +    if (offset < 0 || offset + len > wcss->mem_size)
>>> +        return NULL;
>>> +
>>> +    return wcss->mem_region + offset;
>>> +}
>>> +
>>> +static int q6_wcss_load(struct rproc *rproc, const struct firmware *fw)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +    const struct firmware *fw_hdl;
>>> +    int ret;
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int loop;
>>> +
>>> +    ret = qcom_mdt_load(wcss->dev, fw, rproc->firmware,
>>> +                desc->pasid, wcss->mem_region,
>>> +                wcss->mem_phys, wcss->mem_size,
>>> +                &wcss->mem_reloc);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    for (loop = 1; loop < MAX_FIRMWARE; loop++) {
>>> +        if (!wcss->firmware[loop])
>>> +            continue;
>>> +
>>> +        ret = request_firmware(&fw_hdl, wcss->firmware[loop],
>>> +                       wcss->dev);
>>> +        if (ret)
>>> +            continue;
>>> +
>>> +        ret = qcom_mdt_load_no_init(wcss->dev, fw_hdl,
>>> +                        wcss->firmware[loop], 0,
>>> +                        wcss->mem_region,
>>> +                        wcss->mem_phys,
>>> +                        wcss->mem_size,
>>> +                        &wcss->mem_reloc);
>>> +
>>> +        release_firmware(fw_hdl);
>>> +
>>> +        if (ret) {
>>> +            dev_err(wcss->dev,
>>> +                "can't load %s ret:%d\n", wcss->firmware[loop], ret);
>>> +            return ret;
>>> +        }
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +/* This function load's userpd firmware. Since Userpd depends on rootpd
>>
>> Use Linux coding style comments.
>>

	Sure, I will update.

>>> + * first bring up root pd and then load. User pd firmware load is 
>>> required
>>> + * only during user pd restart because root pd loads user pd FW pil 
>>> segments
>>> + * during it's bringup.
>>> + */
>>> +static int wcss_ahb_pcie_pd_load(struct rproc *rproc, const struct 
>>> firmware *fw)
>>> +{
>>> +    struct q6_wcss *wcss = rproc->priv, *wcss_rpd;
>>> +    struct rproc *rpd_rproc = dev_get_drvdata(wcss->dev->parent);
>>> +    const struct wcss_data *desc = wcss->desc;
>>> +    int ret;
>>> +
>>> +    wcss_rpd = rpd_rproc->priv;
>>> +
>>> +    /* Boot rootpd rproc */
>>
>> That's not helpful - i tcopies the function name.
>>

	sure, i will remove comment.

>>> +    ret = rproc_boot(rpd_rproc);
>>> +    if (ret || wcss->state == WCSS_NORMAL)
>>> +        return ret;
>>> +
>>> +    return desc->mdt_load_sec(wcss->dev, fw, rproc->firmware,
>>> +                  desc->pasid, wcss->mem_region,
>>> +                  wcss->mem_phys, wcss->mem_size,
>>> +                  &wcss->mem_reloc);
>>> +}
>>> +

...

	I didn't understand. Can you please elaborate your comment?

>>> +static int q6_get_inbound_irq(struct qcom_q6v5 *q6,
>>> +                  struct platform_device *pdev,
>>> +                  const char *int_name,
>>> +                  irqreturn_t (*handler)(int irq, void *data))
>>> +{
>>> +    int ret, irq;
>>> +    char *interrupt, *tmp = (char *)int_name;
>>> +    struct q6_wcss *wcss = q6->rproc->priv;
>>> +
>>> +    irq = platform_get_irq_byname(pdev, int_name);
>>> +    if (irq < 0) {
>>> +        if (irq != -EPROBE_DEFER)
>>> +            dev_err(&pdev->dev,
>>> +                "failed to retrieve %s IRQ: %d\n",
>>> +                    int_name, irq);
>>
>> return dev_err_probe
>>

	I will use dev_err_probe.

>>> +        return irq;
>>> +    }
>>> +
>>> +    if (!strcmp(int_name, "fatal")) {
>>> +        q6->fatal_irq = irq;
>>> +    } else if (!strcmp(int_name, "stop-ack")) {
>>> +        q6->stop_irq = irq;
>>> +        tmp = "stop_ack";
>>> +    } else if (!strcmp(int_name, "ready")) {
>>> +        q6->ready_irq = irq;
>>> +    } else if (!strcmp(int_name, "handover")) {
>>> +        q6->handover_irq  = irq;
>>> +    } else if (!strcmp(int_name, "spawn-ack")) {
>>> +        q6->spawn_irq = irq;
>>> +        tmp = "spawn_ack";
>>> +    } else {
>>> +        dev_err(&pdev->dev, "unknown interrupt\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> This is over complicated method of getting interrupts. First, you can
>> pass here address of variable with interrupt to assign (*irq_field =
>> irq). Second, drop the names and get by index. Entries are fixed, not
>> flexible.
>>
	Sure, I will do it.

>>> +
>>> +    interrupt = devm_kzalloc(&pdev->dev, BUF_SIZE, GFP_KERNEL);
>>> +    if (!interrupt)
>>> +        return -ENOMEM;
>>> +
>>> +    snprintf(interrupt, BUF_SIZE, "q6v5_wcss_userpd%d_%s", 
>>> wcss->pd_asid, tmp);
>>> +
>>> +    ret = devm_request_threaded_irq(&pdev->dev, irq,
>>> +                    NULL, handler,
>>> +                    IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>>> +                    interrupt, q6);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to acquire %s irq\n", interrupt);
>>> +        return ret;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static int q6_get_outbound_irq(struct qcom_q6v5 *q6,
>>> +                   struct platform_device *pdev,
>>> +                   const char *int_name)
>>> +{
>>> +    struct qcom_smem_state *tmp_state;
>>> +    unsigned  bit;
>>> +
>>> +    tmp_state = qcom_smem_state_get(&pdev->dev, int_name, &bit);
>>> +    if (IS_ERR(tmp_state)) {
>>> +        dev_err(&pdev->dev, "failed to acquire %s state\n", int_name);
>>> +        return PTR_ERR(tmp_state);
>>> +    }
>>> +
>>> +    if (!strcmp(int_name, "stop")) {
>>> +        q6->state = tmp_state;
>>> +        q6->stop_bit = bit;
>>> +    } else if (!strcmp(int_name, "spawn")) {
>>> +        q6->spawn_state = tmp_state;
>>> +        q6->spawn_bit = bit;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int init_irq(struct qcom_q6v5 *q6,
>>> +            struct platform_device *pdev, struct rproc *rproc,
>>> +            int crash_reason, const char *load_state,
>>> +            void (*handover)(struct qcom_q6v5 *q6))
>>> +{
>>> +    int ret;
>>> +
>>> +    q6->rproc = rproc;
>>> +    q6->dev = &pdev->dev;
>>> +    q6->crash_reason = crash_reason;
>>> +    q6->handover = handover;
>>> +
>>> +    init_completion(&q6->start_done);
>>> +    init_completion(&q6->stop_done);
>>> +    init_completion(&q6->spawn_done);
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "fatal",
>>> +                 q6v5_fatal_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "ready",
>>> +                 q6v5_ready_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "stop-ack",
>>> +                 q6v5_stop_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_inbound_irq(q6, pdev, "spawn-ack",
>>> +                 q6v5_spawn_interrupt);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_outbound_irq(q6, pdev, "stop");
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = q6_get_outbound_irq(q6, pdev, "spawn");
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int q6_wcss_probe(struct platform_device *pdev)
>>> +{
>>> +    const struct wcss_data *desc;
>>> +    struct q6_wcss *wcss;
>>> +    struct rproc *rproc;
>>> +    int ret;
>>> +    char *subdev_name;
>>> +    const char **firmware;
>>> +
>>> +    desc = of_device_get_match_data(&pdev->dev);
>>> +    if (!desc)
>>> +        return -EINVAL;
>>> +
>>> +    firmware = devm_kcalloc(&pdev->dev, MAX_FIRMWARE,
>>> +                sizeof(*firmware), GFP_KERNEL);
>>> +    if (!firmware)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = of_property_read_string_array(pdev->dev.of_node, 
>>> "firmware-name",
>>> +                        firmware, MAX_FIRMWARE);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops,
>>> +                firmware[0], sizeof(*wcss));
>>> +    if (!rproc) {
>>> +        dev_err(&pdev->dev, "failed to allocate rproc\n");
>>
>> ENOMEM do not print errors. Why do you have to print something here?
>>

	Yeah, I will remove print.

>>> +        return -ENOMEM;
>>> +    }
>>> +    wcss = rproc->priv;
>>> +    wcss->dev = &pdev->dev;
>>> +    wcss->desc = desc;
>>> +    wcss->firmware = firmware;
>>> +
>>> +    ret = q6_alloc_memory_region(wcss);
>>> +    if (ret)
>>> +        goto free_rproc;
>>> +
>>> +    wcss->pd_asid = qcom_get_pd_asid(wcss->dev->of_node);
>>> +    if (wcss->pd_asid < 0)
>>> +        goto free_rproc;
>>> +
>>> +    if (desc->init_irq) {
>>> +        ret = desc->init_irq(&wcss->q6, pdev, rproc,
>>> +                     desc->crash_reason_smem, NULL, NULL);
>>> +        if (ret)
>>> +            goto free_rproc;
>>> +    }
>>> +
>>> +    if (desc->glink_subdev_required)
>>> +        qcom_add_glink_subdev(rproc, &wcss->glink_subdev, 
>>> desc->ssr_name);
>>> +
>>> +    subdev_name = (char *)(desc->ssr_name ? desc->ssr_name : 
>>> pdev->name);
>>> +    qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, subdev_name);
>>> +
>>> +    rproc->auto_boot = false;
>>> +    ret = rproc_add(rproc);
>>> +    if (ret)
>>> +        goto free_rproc;
>>> +
>>> +    platform_set_drvdata(pdev, rproc);
>>> +
>>> +    ret = of_platform_populate(wcss->dev->of_node, NULL,
>>> +                   NULL, wcss->dev);
>>
>> It is the same probe used for the children, right? So whom do they 
>> populate?
>>

	I will add match table in 2nd param, so that childs, whose
	compatible matches with match table will be populated.

	Is this fine ?
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to populate wcss pd nodes\n");
>>
>> dev_err_probe
>>
	 Sure, i will use dev_err_probe

>>> +        goto free_rproc;
>>> +    }
>>> +    return 0;
>>> +
>>> +free_rproc:
>>> +    rproc_free(rproc);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int q6_wcss_remove(struct platform_device *pdev)
>>> +{
>>> +    struct rproc *rproc = platform_get_drvdata(pdev);
>>> +    struct q6_wcss *wcss = rproc->priv;
>>> +
>>> +    qcom_q6v5_deinit(&wcss->q6);
>>> +
>>> +    rproc_del(rproc);
>>> +    rproc_free(rproc);
>>> +
>>> +    return 0;
>>> +}
>>
>>
>>

Thanks & Regards,
Manikanta.