From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1A5FAC47082 for ; Mon, 7 Jun 2021 16:47:30 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E0A9E60FDA for ; Mon, 7 Jun 2021 16:47:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E0A9E60FDA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=KLIFlIqLeFz9dqO2zQtN+qTf39lkScc+eWGXe6uEWMg=; b=aHu9L4Pv7NuGSz /Z0LD5Z8sDCyIrpGlndaxy10fXbvcHEOZaZlUQPKy2zOXPF1KrRe4/0UAwek2UPye+LKimiaAdX4o 2PFx5YiU62cQGW5m1wuqOYip/XLtDu7Yw5OVRTFr8wLbDxXyDkdcG1CQdyUW7X5NzXrsIFljfS6MW 4XHBj+tmxT5zh3pSa77HFc3Y5R2WF29vXlqsaw/sbk495qCzsSLODE+32ldogI+tfzDowT18XE01b svz9ljkxU/WncAUPL/uRZazKl7/FtzQpmX1C+Z4hiIPpcBeTawHoszN1au+5PifNKe5Sc5BbtA8DY yJHkNuxyd5QcvXXZK0uA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lqIML-004a4T-Cp; Mon, 07 Jun 2021 16:44:18 +0000 Received: from mail-pj1-x102e.google.com ([2607:f8b0:4864:20::102e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lqICE-004XWF-QD for linux-arm-kernel@lists.infradead.org; Mon, 07 Jun 2021 16:33:53 +0000 Received: by mail-pj1-x102e.google.com with SMTP id l10-20020a17090a150ab0290162974722f2so374320pja.2 for ; Mon, 07 Jun 2021 09:33:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=qsd56f3G9j/FDpaEaCv1P//VO9YqnMeV+qDqORL6kys=; b=kHNVAScmjsu31s7RCtDbe4//VCOYQdHspKfTdOOfdwtvmGfJ9nx2sh+qnnHpHtJNNn s5sMnSIpN+UixbQFLWXmgSiHkxySs3ryZUbJWwJBhq+mbwOfpbVuYBxLY+DAqN0iyg0K rvgSOnun+ro1mMasP6oPzB3hzkkGvgZPo1FEvalBClW866gNSFqmfdwn5q+kMydIpOpm fRd0QfKKzXeM6BN5Lvx9t44RStQPLO9nQPmq0o6P2nqoaUFFWORN4vlirSsGj6ngiFzC gwYj20zrVQ667K9l2WP9MMxVHTGZ3Nu6xhtzS8xwZ6AOyxpJCVnAoHgsP4FTIzAklt5U //Rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=qsd56f3G9j/FDpaEaCv1P//VO9YqnMeV+qDqORL6kys=; b=f6+Ul2IhyeQr3/P/FZGWbfA7GaabWVGIRKJFWcOBdE1mipkNIYSidosYtzUCN3vrti 5vkgYFiCdXVPy2Gq+g8rTZDaNowKyY3lOx7Tf0ycLISIlWQpZ0fGBOA/hFmn3CynD1oc QDviGgpZSXSnQyMarenh8knF9/lQKzT29il7AEXqzYd6Nd9e/g+J2pwmqY32+b7mbdtu piGdcXCxhmJ0yNnAb/wyvJgnRy1XwfmW8Hc+Tqsq/Lciyx9TctZAPnqxHQClmMy0vlKc t51s8dQ9YJhmb/ywQlLY2nd7lnPyPN9aBnMZEPIzF2H5cBXsKkeq97olw3R4u2RCvhoD mCXA== X-Gm-Message-State: AOAM530ursNy4+t8XZ7Z89ThDfsn/zP77uNPbt/u35oaDGAl/Rs+uvvm JzI0kGW3D2cUFd65lRkOVOl0Mw== X-Google-Smtp-Source: ABdhPJzhzK5uFxQjoL/yOH/xMGb+WY9WCBJtjBNMK8GtBawvPRfr/YUQh466luCevPZBU361FZSlZQ== X-Received: by 2002:a17:90a:9bc4:: with SMTP id b4mr20937pjw.42.1623083627217; Mon, 07 Jun 2021 09:33:47 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id l8sm9221921pgq.49.2021.06.07.09.33.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Jun 2021 09:33:46 -0700 (PDT) Date: Mon, 7 Jun 2021 10:33:44 -0600 From: Mathieu Poirier To: Suman Anna Cc: Bjorn Andersson , Lokesh Vutla , Tero Kristo , linux-remoteproc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6] remoteproc: k3-dsp: Add support for IPC-only mode for all K3 DSPs Message-ID: <20210607163344.GA292026@p14s> References: <20210522000309.26134-1-s-anna@ti.com> <20210522000309.26134-7-s-anna@ti.com> <20210602160715.GB1797307@xps15> <5ef182f0-cb68-2872-dde2-0ef7b152c92b@ti.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5ef182f0-cb68-2872-dde2-0ef7b152c92b@ti.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210607_093350_933813_CA35B143 X-CRM114-Status: GOOD ( 57.88 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jun 03, 2021 at 09:57:06AM -0500, Suman Anna wrote: > Hi Mathieu, > > On 6/2/21 11:07 AM, Mathieu Poirier wrote: > > On Fri, May 21, 2021 at 07:03:09PM -0500, Suman Anna wrote: > >> Add support to the K3 DSP remoteproc driver to configure all the C66x > >> and C71x cores on J721E SoCs to be either in IPC-only mode or the > >> traditional remoteproc mode. The IPC-only mode expects that the remote > >> processors are already booted by the bootloader, and only perform the > >> minimum steps required to initialize and deinitialize the virtio IPC > >> transports. The remoteproc mode allows the kernel remoteproc driver to > >> do the regular load and boot and other device management operations for > >> a DSP. > >> > >> The IPC-only mode for a DSP is detected and configured at driver probe > >> time by querying the System Firmware for the DSP power and reset state > >> and/or status and making sure that the DSP is indeed started by the > >> bootloaders, otherwise the device is configured for remoteproc mode. > >> > >> Support for IPC-only mode is achieved through .attach(), .detach() and > >> .get_loaded_rsc_table() callback ops and various other flags in both > >> remoteproc core and the K3 DSP remoteproc driver. The resource table > >> follows a design-by-contract approach and is expected to be at the base > >> of the DDR firmware region reserved for each remoteproc, it is mostly > >> expected to contain only the virtio device and trace resource entries. > >> > >> NOTE: > >> The driver cannot configure a DSP core for remoteproc mode by any > >> means without rebooting the kernel if that R5F core has been started > >> by a bootloader. > >> > >> Signed-off-by: Suman Anna > >> --- > >> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 151 ++++++++++++++++++++-- > >> 1 file changed, 138 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > >> index faf60a274e8d..b154a52f1fa6 100644 > >> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > >> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > >> @@ -76,6 +76,7 @@ struct k3_dsp_dev_data { > >> * @ti_sci_id: TI-SCI device identifier > >> * @mbox: mailbox channel handle > >> * @client: mailbox client to request the mailbox channel > >> + * @ipc_only: flag to indicate IPC-only mode > >> */ > >> struct k3_dsp_rproc { > >> struct device *dev; > >> @@ -91,6 +92,7 @@ struct k3_dsp_rproc { > >> u32 ti_sci_id; > >> struct mbox_chan *mbox; > >> struct mbox_client client; > >> + bool ipc_only; > >> }; > >> > >> /** > >> @@ -268,6 +270,10 @@ static int k3_dsp_rproc_prepare(struct rproc *rproc) > >> struct device *dev = kproc->dev; > >> int ret; > >> > >> + /* IPC-only mode does not require the core to be released from reset */ > >> + if (kproc->ipc_only) > >> + return 0; > >> + > >> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci, > >> kproc->ti_sci_id); > >> if (ret) > >> @@ -292,6 +298,10 @@ static int k3_dsp_rproc_unprepare(struct rproc *rproc) > >> struct device *dev = kproc->dev; > >> int ret; > >> > >> + /* do not put back the cores into reset in IPC-only mode */ > >> + if (kproc->ipc_only) > >> + return 0; > >> + > >> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci, > >> kproc->ti_sci_id); > >> if (ret) > >> @@ -314,6 +324,12 @@ static int k3_dsp_rproc_start(struct rproc *rproc) > >> u32 boot_addr; > >> int ret; > >> > >> + if (kproc->ipc_only) { > >> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n", > >> + __func__); > >> + return -EINVAL; > >> + } > >> + > >> ret = k3_dsp_rproc_request_mbox(rproc); > >> if (ret) > >> return ret; > >> @@ -351,6 +367,13 @@ static int k3_dsp_rproc_start(struct rproc *rproc) > >> static int k3_dsp_rproc_stop(struct rproc *rproc) > >> { > >> struct k3_dsp_rproc *kproc = rproc->priv; > >> + struct device *dev = kproc->dev; > >> + > >> + if (kproc->ipc_only) { > >> + dev_err(dev, "%s cannot be invoked in IPC-only mode\n", > >> + __func__); > >> + return -EINVAL; > >> + } > >> > >> mbox_free_channel(kproc->mbox); > >> > >> @@ -359,6 +382,85 @@ static int k3_dsp_rproc_stop(struct rproc *rproc) > >> return 0; > >> } > >> > >> +/* > >> + * Attach to a running DSP remote processor (IPC-only mode) > >> + * > >> + * This rproc attach callback only needs to request the mailbox, the remote > >> + * processor is already booted, so there is no need to issue any TI-SCI > >> + * commands to boot the DSP core. > >> + */ > >> +static int k3_dsp_rproc_attach(struct rproc *rproc) > >> +{ > >> + struct k3_dsp_rproc *kproc = rproc->priv; > >> + struct device *dev = kproc->dev; > >> + int ret; > >> + > >> + if (!kproc->ipc_only || rproc->state != RPROC_DETACHED) { > >> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_DETACHED state\n"); > >> + return -EINVAL; > >> + } > >> + > >> + ret = k3_dsp_rproc_request_mbox(rproc); > >> + if (ret) > >> + return ret; > >> + > >> + dev_err(dev, "DSP initialized in IPC-only mode\n"); > >> + return 0; > >> +} > >> + > >> +/* > >> + * Detach from a running DSP remote processor (IPC-only mode) > >> + * > >> + * This rproc detach callback performs the opposite operation to attach callback > >> + * and only needs to release the mailbox, the DSP core is not stopped and will > >> + * be left to continue to run its booted firmware. > >> + */ > >> +static int k3_dsp_rproc_detach(struct rproc *rproc) > >> +{ > >> + struct k3_dsp_rproc *kproc = rproc->priv; > >> + struct device *dev = kproc->dev; > >> + > >> + if (!kproc->ipc_only || rproc->state != RPROC_ATTACHED) { > >> + dev_err(dev, "DSP is expected to be in IPC-only mode and RPROC_ATTACHED state\n"); > >> + return -EINVAL; > >> + } > >> + > >> + mbox_free_channel(kproc->mbox); > >> + dev_err(dev, "DSP deinitialized in IPC-only mode\n"); > >> + return 0; > >> +} > > > > Same comment as patch 4/6 regarding k3_dsp_rproc::ipc_only and setting the right > > rproc_ops based on the scenario. > > OK, I will make the switch since both you and Bjorn prefer it this way. And we > can revisit later when we want to scale it to support proper shutdown as well. > FWIW, I have given my reasons for doing it the current way in a previous > response to Bjorn for similar comments. > > Note that I won't be able to define a separate ops structure but rather > overwrite the ops upon detection of IPC-only mode, since I won't be able to > detect the mode until I parse the dt and query our central system processor. The > dt parsing is all done post rproc_alloc, and I need to supply a rproc_ops first. As far as I can tell for both R5 and DSP, rproc_alloc() could be done after the system processor is probed. What am I missing? > > Btw, any comments on patch 1 which is just a cleanup patch. That patch perplexed me. If ops->detach() is not implemented and the detach process is allowed to carry on, how does the remote processor know the remoteproc core is no longer available? > > regards > Suman > > > > > Thanks, > > Mathieu > > > >> + > >> +/* > >> + * This function implements the .get_loaded_rsc_table() callback and is used > >> + * to provide the resource table for a booted DSP in IPC-only mode. The K3 DSP > >> + * firmwares follow a design-by-contract approach and are expected to have the > >> + * resource table at the base of the DDR region reserved for firmware usage. > >> + * This provides flexibility for the remote processor to be booted by different > >> + * bootloaders that may or may not have the ability to publish the resource table > >> + * address and size through a DT property. > >> + */ > >> +static struct resource_table *k3_dsp_get_loaded_rsc_table(struct rproc *rproc, > >> + size_t *rsc_table_sz) > >> +{ > >> + struct k3_dsp_rproc *kproc = rproc->priv; > >> + struct device *dev = kproc->dev; > >> + > >> + if (!kproc->rmem[0].cpu_addr) { > >> + dev_err(dev, "memory-region #1 does not exist, loaded rsc table can't be found"); > >> + return ERR_PTR(-ENOMEM); > >> + } > >> + > >> + /* > >> + * NOTE: The resource table size is currently hard-coded to a maximum > >> + * of 256 bytes. The most common resource table usage for K3 firmwares > >> + * is to only have the vdev resource entry and an optional trace entry. > >> + * The exact size could be computed based on resource table address, but > >> + * the hard-coded value suffices to support the IPC-only mode. > >> + */ > >> + *rsc_table_sz = 256; > >> + return (struct resource_table *)kproc->rmem[0].cpu_addr; > >> +} > >> + > >> /* > >> * Custom function to translate a DSP device address (internal RAMs only) to a > >> * kernel virtual address. The DSPs can access their RAMs at either an internal > >> @@ -421,8 +523,11 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool > >> static const struct rproc_ops k3_dsp_rproc_ops = { > >> .start = k3_dsp_rproc_start, > >> .stop = k3_dsp_rproc_stop, > >> + .attach = k3_dsp_rproc_attach, > >> + .detach = k3_dsp_rproc_detach, > >> .kick = k3_dsp_rproc_kick, > >> .da_to_va = k3_dsp_rproc_da_to_va, > >> + .get_loaded_rsc_table = k3_dsp_get_loaded_rsc_table, > >> }; > >> > >> static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev, > >> @@ -605,6 +710,8 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev) > >> struct k3_dsp_rproc *kproc; > >> struct rproc *rproc; > >> const char *fw_name; > >> + bool r_state = false; > >> + bool p_state = false; > >> int ret = 0; > >> int ret1; > >> > >> @@ -683,19 +790,37 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev) > >> goto release_tsp; > >> } > >> > >> - /* > >> - * ensure the DSP local reset is asserted to ensure the DSP doesn't > >> - * execute bogus code in .prepare() when the module reset is released. > >> - */ > >> - if (data->uses_lreset) { > >> - ret = reset_control_status(kproc->reset); > >> - if (ret < 0) { > >> - dev_err(dev, "failed to get reset status, status = %d\n", > >> - ret); > >> - goto release_mem; > >> - } else if (ret == 0) { > >> - dev_warn(dev, "local reset is deasserted for device\n"); > >> - k3_dsp_rproc_reset(kproc); > >> + ret = kproc->ti_sci->ops.dev_ops.is_on(kproc->ti_sci, kproc->ti_sci_id, > >> + &r_state, &p_state); > >> + if (ret) { > >> + dev_err(dev, "failed to get initial state, mode cannot be determined, ret = %d\n", > >> + ret); > >> + goto release_mem; > >> + } > >> + > >> + /* configure J721E devices for either remoteproc or IPC-only mode */ > >> + if (p_state) { > >> + dev_err(dev, "configured DSP for IPC-only mode\n"); > >> + rproc->state = RPROC_DETACHED; > >> + rproc->detach_on_shutdown = true; > >> + kproc->ipc_only = true; > >> + } else { > >> + dev_err(dev, "configured DSP for remoteproc mode\n"); > >> + /* > >> + * ensure the DSP local reset is asserted to ensure the DSP > >> + * doesn't execute bogus code in .prepare() when the module > >> + * reset is released. > >> + */ > >> + if (data->uses_lreset) { > >> + ret = reset_control_status(kproc->reset); > >> + if (ret < 0) { > >> + dev_err(dev, "failed to get reset status, status = %d\n", > >> + ret); > >> + goto release_mem; > >> + } else if (ret == 0) { > >> + dev_warn(dev, "local reset is deasserted for device\n"); > >> + k3_dsp_rproc_reset(kproc); > >> + } > >> } > >> } > >> > >> -- > >> 2.30.1 > >> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel