Message ID | 20230313111127.1229187-3-danishanwar@ti.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp1120788wrd; Mon, 13 Mar 2023 04:16:42 -0700 (PDT) X-Google-Smtp-Source: AK7set8zzd5MCExXzXmDenXzKmkBDP2vGZtDIcNInDczyfAEQ8jLnMrqIcfVfWu0IEUgYzM5KHzw X-Received: by 2002:a62:1b86:0:b0:5a8:cb5f:94d4 with SMTP id b128-20020a621b86000000b005a8cb5f94d4mr24020648pfb.32.1678706201928; Mon, 13 Mar 2023 04:16:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678706201; cv=none; d=google.com; s=arc-20160816; b=FsrMBGIbWef8F2WSeiTAH8GvPO9t5nT5JXwnuNabz7tMNZEOzWNT9PZxdRdUpXk2K7 rn93Ntorf44nukL+K3yGcytdq6f+cXKMesDtjkWOboQi8J09r0xPfGw5cSY2WCTzaLdJ 73DJ5xJUsy+obLrp8I6WDk4z9HA5Pje0FiGSJpAERIzQITHdE7hdVK15msr9SJAi+ZqW 1D91cq4OtLumEqVXKAJWGDfkAGY3ud82QoaUzJo6vcVk2RDondbMZDsMkTu3RqnR1N+5 AYLdciiz94+YpPmPRZyyMmD3QKKN4Ek+3icT9W20aBfOHePzcirSrCfowMXBpePSlbM6 vZfQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=I2qPeTuoz8w/2Or8w2ppF0jbDDH3044qpWoLVlOVKOg=; b=v0PRTw0djXZKMoseVCh9x7kwCUjENnnJfo2d92IIe9bfpWntGHCvpLsJ4q9Few/BNQ nVjDUF0P6QPZOFgfcO9ROy0+rPJL2VHC0+hKaICHrLdqWMT/AY3T8hP5iMGcc2nrTACA x5M6sITdPx1Ur1XgAH5dqFxC60aCkoRiugN6gsVNM68K2e8VP160Etz2KjnjCZWqgHs4 2/uR9BYr46W2rnhgZDvvHLzo67rGg6Pb1TFAnq0ynvbcdS2OCHhDP8K3sMn/iLHWbilk niJnVcfMMBK3Ii+YnvfxDJW/7v5pPj50TlNBIn1A6rmKWGvZsg0q50UGEhMwDzmEF/Ch kC8Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=wKBtUF7Z; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n13-20020aa7984d000000b005dd190cfc54si6266897pfq.349.2023.03.13.04.16.29; Mon, 13 Mar 2023 04:16:41 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=wKBtUF7Z; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230419AbjCMLNE (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Mon, 13 Mar 2023 07:13:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51822 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230409AbjCMLM1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 13 Mar 2023 07:12:27 -0400 Received: from fllv0015.ext.ti.com (fllv0015.ext.ti.com [198.47.19.141]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4EBB360A95; Mon, 13 Mar 2023 04:11:47 -0700 (PDT) Received: from fllv0034.itg.ti.com ([10.64.40.246]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 32DBBZKI001220; Mon, 13 Mar 2023 06:11:35 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1678705895; bh=I2qPeTuoz8w/2Or8w2ppF0jbDDH3044qpWoLVlOVKOg=; h=From:To:CC:Subject:Date:In-Reply-To:References; b=wKBtUF7ZDs7Mbq/EjBC/ZTLNzifvHGSMIiJtTmOHu0Hk0EQt0Jae755pBfqMLwf7q /y5WhsgitPvU900EA3pW5pr16ziTveuq6yiSMw5K4mJqdpZdxr0mrIcY4GJSiuD5kM euvoimm2PVEi1EmhyBUB21CSHMn3V42pvJachzxU= Received: from DFLE113.ent.ti.com (dfle113.ent.ti.com [10.64.6.34]) by fllv0034.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 32DBBZSD014991 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 13 Mar 2023 06:11:35 -0500 Received: from DFLE115.ent.ti.com (10.64.6.36) by DFLE113.ent.ti.com (10.64.6.34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16; Mon, 13 Mar 2023 06:11:35 -0500 Received: from lelv0326.itg.ti.com (10.180.67.84) by DFLE115.ent.ti.com (10.64.6.36) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16 via Frontend Transport; Mon, 13 Mar 2023 06:11:34 -0500 Received: from lelv0854.itg.ti.com (lelv0854.itg.ti.com [10.181.64.140]) by lelv0326.itg.ti.com (8.15.2/8.15.2) with ESMTP id 32DBBYbM016815; Mon, 13 Mar 2023 06:11:34 -0500 Received: from localhost (a0501179-pc.dhcp.ti.com [10.24.69.114]) by lelv0854.itg.ti.com (8.14.7/8.14.7) with ESMTP id 32DBBXJJ032269; Mon, 13 Mar 2023 06:11:34 -0500 From: MD Danish Anwar <danishanwar@ti.com> To: "Andrew F. Davis" <afd@ti.com>, Suman Anna <s-anna@ti.com>, Roger Quadros <rogerq@kernel.org>, Vignesh Raghavendra <vigneshr@ti.com>, MD Danish Anwar <danishanwar@ti.com>, Mathieu Poirier <mathieu.poirier@linaro.org>, Bjorn Andersson <andersson@kernel.org>, Santosh Shilimkar <ssantosh@kernel.org>, Nishanth Menon <nm@ti.com> CC: <linux-remoteproc@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>, <srk@ti.com>, <devicetree@vger.kernel.org>, <netdev@vger.kernel.org> Subject: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API Date: Mon, 13 Mar 2023 16:41:24 +0530 Message-ID: <20230313111127.1229187-3-danishanwar@ti.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230313111127.1229187-1-danishanwar@ti.com> References: <20230313111127.1229187-1-danishanwar@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760251034450697738?= X-GMAIL-MSGID: =?utf-8?q?1760251034450697738?= |
Series |
Introduce PRU platform consumer API
|
|
Commit Message
MD Danish Anwar
March 13, 2023, 11:11 a.m. UTC
From: "Andrew F. Davis" <afd@ti.com> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), to the PRUSS platform driver to allow client drivers to acquire and release the common memory resources present within a PRU-ICSS subsystem. This allows the client drivers to directly manipulate the respective memories, as per their design contract with the associated firmware. Co-developed-by: Suman Anna <s-anna@ti.com> Signed-off-by: Suman Anna <s-anna@ti.com> Signed-off-by: Andrew F. Davis <afd@ti.com> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> Reviewed-by: Roger Quadros <rogerq@kernel.org> --- drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ include/linux/pruss_driver.h | 27 +++-------- include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ 3 files changed, 121 insertions(+), 22 deletions(-)
Comments
Hi Andrew & Danish, On 13/03/2023 13:11, MD Danish Anwar wrote: > From: "Andrew F. Davis" <afd@ti.com> > > Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), > to the PRUSS platform driver to allow client drivers to acquire and release > the common memory resources present within a PRU-ICSS subsystem. This > allows the client drivers to directly manipulate the respective memories, > as per their design contract with the associated firmware. > > Co-developed-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Andrew F. Davis <afd@ti.com> > Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> > Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> > Signed-off-by: MD Danish Anwar <danishanwar@ti.com> > Reviewed-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ > include/linux/pruss_driver.h | 27 +++-------- > include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ We have these 2 header files and I think anything that deals with 'struct pruss' should go in include/linux/pruss_driver.h Anything that deals with pru_rproc (i.e. struct rproc) should go in include/linux/remoteproc/pruss.h Do you agree? > 3 files changed, 121 insertions(+), 22 deletions(-) > > diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c > index a169aa1ed044..c8053c0d735f 100644 > --- a/drivers/soc/ti/pruss.c > +++ b/drivers/soc/ti/pruss.c > @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) > } > EXPORT_SYMBOL_GPL(pruss_put); > > +/** > + * pruss_request_mem_region() - request a memory resource > + * @pruss: the pruss instance > + * @mem_id: the memory resource id > + * @region: pointer to memory region structure to be filled in > + * > + * This function allows a client driver to request a memory resource, > + * and if successful, will let the client driver own the particular > + * memory region until released using the pruss_release_mem_region() > + * API. > + * > + * Return: 0 if requested memory region is available (in such case pointer to > + * memory region is returned via @region), an error otherwise > + */ > +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, > + struct pruss_mem_region *region) > +{ > + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) > + return -EINVAL; > + > + mutex_lock(&pruss->lock); > + > + if (pruss->mem_in_use[mem_id]) { > + mutex_unlock(&pruss->lock); > + return -EBUSY; > + } > + > + *region = pruss->mem_regions[mem_id]; > + pruss->mem_in_use[mem_id] = region; > + > + mutex_unlock(&pruss->lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pruss_request_mem_region); > + > +/** > + * pruss_release_mem_region() - release a memory resource > + * @pruss: the pruss instance > + * @region: the memory region to release > + * > + * This function is the complimentary function to > + * pruss_request_mem_region(), and allows the client drivers to > + * release back a memory resource. > + * > + * Return: 0 on success, an error code otherwise > + */ > +int pruss_release_mem_region(struct pruss *pruss, > + struct pruss_mem_region *region) > +{ > + int id; > + > + if (!pruss || !region) > + return -EINVAL; > + > + mutex_lock(&pruss->lock); > + > + /* find out the memory region being released */ > + for (id = 0; id < PRUSS_MEM_MAX; id++) { > + if (pruss->mem_in_use[id] == region) > + break; > + } > + > + if (id == PRUSS_MEM_MAX) { > + mutex_unlock(&pruss->lock); > + return -EINVAL; > + } > + > + pruss->mem_in_use[id] = NULL; > + > + mutex_unlock(&pruss->lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pruss_release_mem_region); > + > static void pruss_of_free_clk_provider(void *data) > { > struct device_node *clk_mux_np = data; > @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) > return -ENOMEM; > > pruss->dev = dev; > + mutex_init(&pruss->lock); > > child = of_get_child_by_name(np, "memories"); > if (!child) { > diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h > index 86242fb5a64a..22b4b37d2536 100644 > --- a/include/linux/pruss_driver.h > +++ b/include/linux/pruss_driver.h > @@ -9,37 +9,18 @@ > #ifndef _PRUSS_DRIVER_H_ > #define _PRUSS_DRIVER_H_ > > +#include <linux/mutex.h> > #include <linux/remoteproc/pruss.h> > #include <linux/types.h> > > -/* > - * enum pruss_mem - PRUSS memory range identifiers > - */ > -enum pruss_mem { > - PRUSS_MEM_DRAM0 = 0, > - PRUSS_MEM_DRAM1, > - PRUSS_MEM_SHRD_RAM2, > - PRUSS_MEM_MAX, > -}; > - > -/** > - * struct pruss_mem_region - PRUSS memory region structure > - * @va: kernel virtual address of the PRUSS memory region > - * @pa: physical (bus) address of the PRUSS memory region > - * @size: size of the PRUSS memory region > - */ > -struct pruss_mem_region { > - void __iomem *va; > - phys_addr_t pa; > - size_t size; > -}; > - > /** > * struct pruss - PRUSS parent structure > * @dev: pruss device pointer > * @cfg_base: base iomap for CFG region > * @cfg_regmap: regmap for config region > * @mem_regions: data for each of the PRUSS memory regions > + * @mem_in_use: to indicate if memory resource is in use > + * @lock: mutex to serialize access to resources > * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX > * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX > */ > @@ -48,6 +29,8 @@ struct pruss { > void __iomem *cfg_base; > struct regmap *cfg_regmap; > struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; > + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; > + struct mutex lock; /* PRU resource lock */ > struct clk *core_clk_mux; > struct clk *iep_clk_mux; > }; > diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h > index 93a98cac7829..33f930e0a0ce 100644 > --- a/include/linux/remoteproc/pruss.h > +++ b/include/linux/remoteproc/pruss.h > @@ -44,6 +44,28 @@ enum pru_ctable_idx { > PRU_C31, > }; > > +/* > + * enum pruss_mem - PRUSS memory range identifiers > + */ > +enum pruss_mem { > + PRUSS_MEM_DRAM0 = 0, > + PRUSS_MEM_DRAM1, > + PRUSS_MEM_SHRD_RAM2, > + PRUSS_MEM_MAX, > +}; > + > +/** > + * struct pruss_mem_region - PRUSS memory region structure > + * @va: kernel virtual address of the PRUSS memory region > + * @pa: physical (bus) address of the PRUSS memory region > + * @size: size of the PRUSS memory region > + */ > +struct pruss_mem_region { > + void __iomem *va; > + phys_addr_t pa; > + size_t size; > +}; > + > struct device_node; > struct rproc; > struct pruss; > @@ -52,6 +74,10 @@ struct pruss; > > struct pruss *pruss_get(struct rproc *rproc); > void pruss_put(struct pruss *pruss); > +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, > + struct pruss_mem_region *region); > +int pruss_release_mem_region(struct pruss *pruss, > + struct pruss_mem_region *region); > > #else > > @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc *rproc) > > static inline void pruss_put(struct pruss *pruss) { } > > +static inline int pruss_request_mem_region(struct pruss *pruss, > + enum pruss_mem mem_id, > + struct pruss_mem_region *region) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline int pruss_release_mem_region(struct pruss *pruss, > + struct pruss_mem_region *region) > +{ > + return -EOPNOTSUPP; > +} > + > #endif /* CONFIG_TI_PRUSS */ > > #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) cheers, -roger
Hi Roger, On 17/03/23 14:26, Roger Quadros wrote: > Hi Andrew & Danish, > > > On 13/03/2023 13:11, MD Danish Anwar wrote: >> From: "Andrew F. Davis" <afd@ti.com> >> >> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >> to the PRUSS platform driver to allow client drivers to acquire and release >> the common memory resources present within a PRU-ICSS subsystem. This >> allows the client drivers to directly manipulate the respective memories, >> as per their design contract with the associated firmware. >> >> Co-developed-by: Suman Anna <s-anna@ti.com> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >> Reviewed-by: Roger Quadros <rogerq@kernel.org> >> --- >> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >> include/linux/pruss_driver.h | 27 +++-------- >> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ > > > We have these 2 header files and I think anything that deals with > 'struct pruss' should go in include/linux/pruss_driver.h > > Anything that deals with pru_rproc (i.e. struct rproc) should go in > include/linux/remoteproc/pruss.h > > Do you agree? > I agree with you Roger but Andrew is the right person to comment here as he is the author of this and several other patches. Hi Andrew, Can you please comment on this? >> 3 files changed, 121 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >> index a169aa1ed044..c8053c0d735f 100644 >> --- a/drivers/soc/ti/pruss.c >> +++ b/drivers/soc/ti/pruss.c >> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >> } >> EXPORT_SYMBOL_GPL(pruss_put); >> >> +/** >> + * pruss_request_mem_region() - request a memory resource >> + * @pruss: the pruss instance >> + * @mem_id: the memory resource id >> + * @region: pointer to memory region structure to be filled in >> + * >> + * This function allows a client driver to request a memory resource, >> + * and if successful, will let the client driver own the particular >> + * memory region until released using the pruss_release_mem_region() >> + * API. >> + * >> + * Return: 0 if requested memory region is available (in such case pointer to >> + * memory region is returned via @region), an error otherwise >> + */ >> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >> + struct pruss_mem_region *region) >> +{ >> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >> + return -EINVAL; >> + >> + mutex_lock(&pruss->lock); >> + >> + if (pruss->mem_in_use[mem_id]) { >> + mutex_unlock(&pruss->lock); >> + return -EBUSY; >> + } >> + >> + *region = pruss->mem_regions[mem_id]; >> + pruss->mem_in_use[mem_id] = region; >> + >> + mutex_unlock(&pruss->lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >> + >> +/** >> + * pruss_release_mem_region() - release a memory resource >> + * @pruss: the pruss instance >> + * @region: the memory region to release >> + * >> + * This function is the complimentary function to >> + * pruss_request_mem_region(), and allows the client drivers to >> + * release back a memory resource. >> + * >> + * Return: 0 on success, an error code otherwise >> + */ >> +int pruss_release_mem_region(struct pruss *pruss, >> + struct pruss_mem_region *region) >> +{ >> + int id; >> + >> + if (!pruss || !region) >> + return -EINVAL; >> + >> + mutex_lock(&pruss->lock); >> + >> + /* find out the memory region being released */ >> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >> + if (pruss->mem_in_use[id] == region) >> + break; >> + } >> + >> + if (id == PRUSS_MEM_MAX) { >> + mutex_unlock(&pruss->lock); >> + return -EINVAL; >> + } >> + >> + pruss->mem_in_use[id] = NULL; >> + >> + mutex_unlock(&pruss->lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >> + >> static void pruss_of_free_clk_provider(void *data) >> { >> struct device_node *clk_mux_np = data; >> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >> return -ENOMEM; >> >> pruss->dev = dev; >> + mutex_init(&pruss->lock); >> >> child = of_get_child_by_name(np, "memories"); >> if (!child) { >> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >> index 86242fb5a64a..22b4b37d2536 100644 >> --- a/include/linux/pruss_driver.h >> +++ b/include/linux/pruss_driver.h >> @@ -9,37 +9,18 @@ >> #ifndef _PRUSS_DRIVER_H_ >> #define _PRUSS_DRIVER_H_ >> >> +#include <linux/mutex.h> >> #include <linux/remoteproc/pruss.h> >> #include <linux/types.h> >> >> -/* >> - * enum pruss_mem - PRUSS memory range identifiers >> - */ >> -enum pruss_mem { >> - PRUSS_MEM_DRAM0 = 0, >> - PRUSS_MEM_DRAM1, >> - PRUSS_MEM_SHRD_RAM2, >> - PRUSS_MEM_MAX, >> -}; >> - >> -/** >> - * struct pruss_mem_region - PRUSS memory region structure >> - * @va: kernel virtual address of the PRUSS memory region >> - * @pa: physical (bus) address of the PRUSS memory region >> - * @size: size of the PRUSS memory region >> - */ >> -struct pruss_mem_region { >> - void __iomem *va; >> - phys_addr_t pa; >> - size_t size; >> -}; >> - >> /** >> * struct pruss - PRUSS parent structure >> * @dev: pruss device pointer >> * @cfg_base: base iomap for CFG region >> * @cfg_regmap: regmap for config region >> * @mem_regions: data for each of the PRUSS memory regions >> + * @mem_in_use: to indicate if memory resource is in use >> + * @lock: mutex to serialize access to resources >> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >> */ >> @@ -48,6 +29,8 @@ struct pruss { >> void __iomem *cfg_base; >> struct regmap *cfg_regmap; >> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >> + struct mutex lock; /* PRU resource lock */ >> struct clk *core_clk_mux; >> struct clk *iep_clk_mux; >> }; >> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h >> index 93a98cac7829..33f930e0a0ce 100644 >> --- a/include/linux/remoteproc/pruss.h >> +++ b/include/linux/remoteproc/pruss.h >> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >> PRU_C31, >> }; >> >> +/* >> + * enum pruss_mem - PRUSS memory range identifiers >> + */ >> +enum pruss_mem { >> + PRUSS_MEM_DRAM0 = 0, >> + PRUSS_MEM_DRAM1, >> + PRUSS_MEM_SHRD_RAM2, >> + PRUSS_MEM_MAX, >> +}; >> + >> +/** >> + * struct pruss_mem_region - PRUSS memory region structure >> + * @va: kernel virtual address of the PRUSS memory region >> + * @pa: physical (bus) address of the PRUSS memory region >> + * @size: size of the PRUSS memory region >> + */ >> +struct pruss_mem_region { >> + void __iomem *va; >> + phys_addr_t pa; >> + size_t size; >> +}; >> + >> struct device_node; >> struct rproc; >> struct pruss; >> @@ -52,6 +74,10 @@ struct pruss; >> >> struct pruss *pruss_get(struct rproc *rproc); >> void pruss_put(struct pruss *pruss); >> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >> + struct pruss_mem_region *region); >> +int pruss_release_mem_region(struct pruss *pruss, >> + struct pruss_mem_region *region); >> >> #else >> >> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc *rproc) >> >> static inline void pruss_put(struct pruss *pruss) { } >> >> +static inline int pruss_request_mem_region(struct pruss *pruss, >> + enum pruss_mem mem_id, >> + struct pruss_mem_region *region) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +static inline int pruss_release_mem_region(struct pruss *pruss, >> + struct pruss_mem_region *region) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> #endif /* CONFIG_TI_PRUSS */ >> >> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) > > cheers, > -roger
On 3/20/23 12:11 AM, Md Danish Anwar wrote: > Hi Roger, > > On 17/03/23 14:26, Roger Quadros wrote: >> Hi Andrew & Danish, >> >> >> On 13/03/2023 13:11, MD Danish Anwar wrote: >>> From: "Andrew F. Davis" <afd@ti.com> >>> >>> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >>> to the PRUSS platform driver to allow client drivers to acquire and release >>> the common memory resources present within a PRU-ICSS subsystem. This >>> allows the client drivers to directly manipulate the respective memories, >>> as per their design contract with the associated firmware. >>> >>> Co-developed-by: Suman Anna <s-anna@ti.com> >>> Signed-off-by: Suman Anna <s-anna@ti.com> >>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>> --- >>> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >>> include/linux/pruss_driver.h | 27 +++-------- >>> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ >> >> >> We have these 2 header files and I think anything that deals with >> 'struct pruss' should go in include/linux/pruss_driver.h >> >> Anything that deals with pru_rproc (i.e. struct rproc) should go in >> include/linux/remoteproc/pruss.h >> >> Do you agree? >> > > I agree with you Roger but Andrew is the right person to comment here as he is > the author of this and several other patches. > > Hi Andrew, Can you please comment on this? > Original idea was a consumer driver (like "ICSSG Ethernet Driver" in your other series) could just #include <linux/remoteproc/pruss.h> and get everything they need, and nothing they do not. pruss_driver.h (which could be renamed pruss_internal.h) exists to allow comunication between the pruss core and the pru rproc driver which live in different subsystems. Andrew >>> 3 files changed, 121 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>> index a169aa1ed044..c8053c0d735f 100644 >>> --- a/drivers/soc/ti/pruss.c >>> +++ b/drivers/soc/ti/pruss.c >>> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >>> } >>> EXPORT_SYMBOL_GPL(pruss_put); >>> >>> +/** >>> + * pruss_request_mem_region() - request a memory resource >>> + * @pruss: the pruss instance >>> + * @mem_id: the memory resource id >>> + * @region: pointer to memory region structure to be filled in >>> + * >>> + * This function allows a client driver to request a memory resource, >>> + * and if successful, will let the client driver own the particular >>> + * memory region until released using the pruss_release_mem_region() >>> + * API. >>> + * >>> + * Return: 0 if requested memory region is available (in such case pointer to >>> + * memory region is returned via @region), an error otherwise >>> + */ >>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>> + struct pruss_mem_region *region) >>> +{ >>> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >>> + return -EINVAL; >>> + >>> + mutex_lock(&pruss->lock); >>> + >>> + if (pruss->mem_in_use[mem_id]) { >>> + mutex_unlock(&pruss->lock); >>> + return -EBUSY; >>> + } >>> + >>> + *region = pruss->mem_regions[mem_id]; >>> + pruss->mem_in_use[mem_id] = region; >>> + >>> + mutex_unlock(&pruss->lock); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >>> + >>> +/** >>> + * pruss_release_mem_region() - release a memory resource >>> + * @pruss: the pruss instance >>> + * @region: the memory region to release >>> + * >>> + * This function is the complimentary function to >>> + * pruss_request_mem_region(), and allows the client drivers to >>> + * release back a memory resource. >>> + * >>> + * Return: 0 on success, an error code otherwise >>> + */ >>> +int pruss_release_mem_region(struct pruss *pruss, >>> + struct pruss_mem_region *region) >>> +{ >>> + int id; >>> + >>> + if (!pruss || !region) >>> + return -EINVAL; >>> + >>> + mutex_lock(&pruss->lock); >>> + >>> + /* find out the memory region being released */ >>> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >>> + if (pruss->mem_in_use[id] == region) >>> + break; >>> + } >>> + >>> + if (id == PRUSS_MEM_MAX) { >>> + mutex_unlock(&pruss->lock); >>> + return -EINVAL; >>> + } >>> + >>> + pruss->mem_in_use[id] = NULL; >>> + >>> + mutex_unlock(&pruss->lock); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >>> + >>> static void pruss_of_free_clk_provider(void *data) >>> { >>> struct device_node *clk_mux_np = data; >>> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >>> return -ENOMEM; >>> >>> pruss->dev = dev; >>> + mutex_init(&pruss->lock); >>> >>> child = of_get_child_by_name(np, "memories"); >>> if (!child) { >>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >>> index 86242fb5a64a..22b4b37d2536 100644 >>> --- a/include/linux/pruss_driver.h >>> +++ b/include/linux/pruss_driver.h >>> @@ -9,37 +9,18 @@ >>> #ifndef _PRUSS_DRIVER_H_ >>> #define _PRUSS_DRIVER_H_ >>> >>> +#include <linux/mutex.h> >>> #include <linux/remoteproc/pruss.h> >>> #include <linux/types.h> >>> >>> -/* >>> - * enum pruss_mem - PRUSS memory range identifiers >>> - */ >>> -enum pruss_mem { >>> - PRUSS_MEM_DRAM0 = 0, >>> - PRUSS_MEM_DRAM1, >>> - PRUSS_MEM_SHRD_RAM2, >>> - PRUSS_MEM_MAX, >>> -}; >>> - >>> -/** >>> - * struct pruss_mem_region - PRUSS memory region structure >>> - * @va: kernel virtual address of the PRUSS memory region >>> - * @pa: physical (bus) address of the PRUSS memory region >>> - * @size: size of the PRUSS memory region >>> - */ >>> -struct pruss_mem_region { >>> - void __iomem *va; >>> - phys_addr_t pa; >>> - size_t size; >>> -}; >>> - >>> /** >>> * struct pruss - PRUSS parent structure >>> * @dev: pruss device pointer >>> * @cfg_base: base iomap for CFG region >>> * @cfg_regmap: regmap for config region >>> * @mem_regions: data for each of the PRUSS memory regions >>> + * @mem_in_use: to indicate if memory resource is in use >>> + * @lock: mutex to serialize access to resources >>> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >>> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >>> */ >>> @@ -48,6 +29,8 @@ struct pruss { >>> void __iomem *cfg_base; >>> struct regmap *cfg_regmap; >>> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >>> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >>> + struct mutex lock; /* PRU resource lock */ >>> struct clk *core_clk_mux; >>> struct clk *iep_clk_mux; >>> }; >>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h >>> index 93a98cac7829..33f930e0a0ce 100644 >>> --- a/include/linux/remoteproc/pruss.h >>> +++ b/include/linux/remoteproc/pruss.h >>> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >>> PRU_C31, >>> }; >>> >>> +/* >>> + * enum pruss_mem - PRUSS memory range identifiers >>> + */ >>> +enum pruss_mem { >>> + PRUSS_MEM_DRAM0 = 0, >>> + PRUSS_MEM_DRAM1, >>> + PRUSS_MEM_SHRD_RAM2, >>> + PRUSS_MEM_MAX, >>> +}; >>> + >>> +/** >>> + * struct pruss_mem_region - PRUSS memory region structure >>> + * @va: kernel virtual address of the PRUSS memory region >>> + * @pa: physical (bus) address of the PRUSS memory region >>> + * @size: size of the PRUSS memory region >>> + */ >>> +struct pruss_mem_region { >>> + void __iomem *va; >>> + phys_addr_t pa; >>> + size_t size; >>> +}; >>> + >>> struct device_node; >>> struct rproc; >>> struct pruss; >>> @@ -52,6 +74,10 @@ struct pruss; >>> >>> struct pruss *pruss_get(struct rproc *rproc); >>> void pruss_put(struct pruss *pruss); >>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>> + struct pruss_mem_region *region); >>> +int pruss_release_mem_region(struct pruss *pruss, >>> + struct pruss_mem_region *region); >>> >>> #else >>> >>> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc *rproc) >>> >>> static inline void pruss_put(struct pruss *pruss) { } >>> >>> +static inline int pruss_request_mem_region(struct pruss *pruss, >>> + enum pruss_mem mem_id, >>> + struct pruss_mem_region *region) >>> +{ >>> + return -EOPNOTSUPP; >>> +} >>> + >>> +static inline int pruss_release_mem_region(struct pruss *pruss, >>> + struct pruss_mem_region *region) >>> +{ >>> + return -EOPNOTSUPP; >>> +} >>> + >>> #endif /* CONFIG_TI_PRUSS */ >>> >>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) >> >> cheers, >> -roger >
Hi Andrew, Roger, On 20/03/23 21:48, Andrew Davis wrote: > On 3/20/23 12:11 AM, Md Danish Anwar wrote: >> Hi Roger, >> >> On 17/03/23 14:26, Roger Quadros wrote: >>> Hi Andrew & Danish, >>> >>> >>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>> From: "Andrew F. Davis" <afd@ti.com> >>>> >>>> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >>>> to the PRUSS platform driver to allow client drivers to acquire and release >>>> the common memory resources present within a PRU-ICSS subsystem. This >>>> allows the client drivers to directly manipulate the respective memories, >>>> as per their design contract with the associated firmware. >>>> >>>> Co-developed-by: Suman Anna <s-anna@ti.com> >>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>> --- >>>> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >>>> include/linux/pruss_driver.h | 27 +++-------- >>>> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ >>> >>> >>> We have these 2 header files and I think anything that deals with >>> 'struct pruss' should go in include/linux/pruss_driver.h >>> >>> Anything that deals with pru_rproc (i.e. struct rproc) should go in >>> include/linux/remoteproc/pruss.h >>> >>> Do you agree? >>> >> >> I agree with you Roger but Andrew is the right person to comment here as he is >> the author of this and several other patches. >> >> Hi Andrew, Can you please comment on this? >> > > Original idea was a consumer driver (like "ICSSG Ethernet Driver" in your other > series) could just > > #include <linux/remoteproc/pruss.h> > > and get everything they need, and nothing they do not. > If we plan on continuing the original idea, then I think keeping the header files as it is will be the best. Because if we move anything that deals with 'struct pruss' to include/linux/pruss_driver.h and anything that deals with pru_rproc (i.e. struct rproc) to include/linux/remoteproc/pruss.h, then the consumer drivers will need to do, #include <linux/remoteproc/pruss.h> #include <linux/pruss_driver.h> Roger, should I keep the header files arrangement as it is? > pruss_driver.h (which could be renamed pruss_internal.h) exists to allow > comunication between the pruss core and the pru rproc driver which live > in different subsystems. > > Andrew > >>>> 3 files changed, 121 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>> index a169aa1ed044..c8053c0d735f 100644 >>>> --- a/drivers/soc/ti/pruss.c >>>> +++ b/drivers/soc/ti/pruss.c >>>> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >>>> } >>>> EXPORT_SYMBOL_GPL(pruss_put); >>>> +/** >>>> + * pruss_request_mem_region() - request a memory resource >>>> + * @pruss: the pruss instance >>>> + * @mem_id: the memory resource id >>>> + * @region: pointer to memory region structure to be filled in >>>> + * >>>> + * This function allows a client driver to request a memory resource, >>>> + * and if successful, will let the client driver own the particular >>>> + * memory region until released using the pruss_release_mem_region() >>>> + * API. >>>> + * >>>> + * Return: 0 if requested memory region is available (in such case pointer to >>>> + * memory region is returned via @region), an error otherwise >>>> + */ >>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>> + struct pruss_mem_region *region) >>>> +{ >>>> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >>>> + return -EINVAL; >>>> + >>>> + mutex_lock(&pruss->lock); >>>> + >>>> + if (pruss->mem_in_use[mem_id]) { >>>> + mutex_unlock(&pruss->lock); >>>> + return -EBUSY; >>>> + } >>>> + >>>> + *region = pruss->mem_regions[mem_id]; >>>> + pruss->mem_in_use[mem_id] = region; >>>> + >>>> + mutex_unlock(&pruss->lock); >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >>>> + >>>> +/** >>>> + * pruss_release_mem_region() - release a memory resource >>>> + * @pruss: the pruss instance >>>> + * @region: the memory region to release >>>> + * >>>> + * This function is the complimentary function to >>>> + * pruss_request_mem_region(), and allows the client drivers to >>>> + * release back a memory resource. >>>> + * >>>> + * Return: 0 on success, an error code otherwise >>>> + */ >>>> +int pruss_release_mem_region(struct pruss *pruss, >>>> + struct pruss_mem_region *region) >>>> +{ >>>> + int id; >>>> + >>>> + if (!pruss || !region) >>>> + return -EINVAL; >>>> + >>>> + mutex_lock(&pruss->lock); >>>> + >>>> + /* find out the memory region being released */ >>>> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >>>> + if (pruss->mem_in_use[id] == region) >>>> + break; >>>> + } >>>> + >>>> + if (id == PRUSS_MEM_MAX) { >>>> + mutex_unlock(&pruss->lock); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + pruss->mem_in_use[id] = NULL; >>>> + >>>> + mutex_unlock(&pruss->lock); >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >>>> + >>>> static void pruss_of_free_clk_provider(void *data) >>>> { >>>> struct device_node *clk_mux_np = data; >>>> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >>>> return -ENOMEM; >>>> pruss->dev = dev; >>>> + mutex_init(&pruss->lock); >>>> child = of_get_child_by_name(np, "memories"); >>>> if (!child) { >>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >>>> index 86242fb5a64a..22b4b37d2536 100644 >>>> --- a/include/linux/pruss_driver.h >>>> +++ b/include/linux/pruss_driver.h >>>> @@ -9,37 +9,18 @@ >>>> #ifndef _PRUSS_DRIVER_H_ >>>> #define _PRUSS_DRIVER_H_ >>>> +#include <linux/mutex.h> >>>> #include <linux/remoteproc/pruss.h> >>>> #include <linux/types.h> >>>> -/* >>>> - * enum pruss_mem - PRUSS memory range identifiers >>>> - */ >>>> -enum pruss_mem { >>>> - PRUSS_MEM_DRAM0 = 0, >>>> - PRUSS_MEM_DRAM1, >>>> - PRUSS_MEM_SHRD_RAM2, >>>> - PRUSS_MEM_MAX, >>>> -}; >>>> - >>>> -/** >>>> - * struct pruss_mem_region - PRUSS memory region structure >>>> - * @va: kernel virtual address of the PRUSS memory region >>>> - * @pa: physical (bus) address of the PRUSS memory region >>>> - * @size: size of the PRUSS memory region >>>> - */ >>>> -struct pruss_mem_region { >>>> - void __iomem *va; >>>> - phys_addr_t pa; >>>> - size_t size; >>>> -}; >>>> - >>>> /** >>>> * struct pruss - PRUSS parent structure >>>> * @dev: pruss device pointer >>>> * @cfg_base: base iomap for CFG region >>>> * @cfg_regmap: regmap for config region >>>> * @mem_regions: data for each of the PRUSS memory regions >>>> + * @mem_in_use: to indicate if memory resource is in use >>>> + * @lock: mutex to serialize access to resources >>>> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >>>> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >>>> */ >>>> @@ -48,6 +29,8 @@ struct pruss { >>>> void __iomem *cfg_base; >>>> struct regmap *cfg_regmap; >>>> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >>>> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >>>> + struct mutex lock; /* PRU resource lock */ >>>> struct clk *core_clk_mux; >>>> struct clk *iep_clk_mux; >>>> }; >>>> diff --git a/include/linux/remoteproc/pruss.h >>>> b/include/linux/remoteproc/pruss.h >>>> index 93a98cac7829..33f930e0a0ce 100644 >>>> --- a/include/linux/remoteproc/pruss.h >>>> +++ b/include/linux/remoteproc/pruss.h >>>> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >>>> PRU_C31, >>>> }; >>>> +/* >>>> + * enum pruss_mem - PRUSS memory range identifiers >>>> + */ >>>> +enum pruss_mem { >>>> + PRUSS_MEM_DRAM0 = 0, >>>> + PRUSS_MEM_DRAM1, >>>> + PRUSS_MEM_SHRD_RAM2, >>>> + PRUSS_MEM_MAX, >>>> +}; >>>> + >>>> +/** >>>> + * struct pruss_mem_region - PRUSS memory region structure >>>> + * @va: kernel virtual address of the PRUSS memory region >>>> + * @pa: physical (bus) address of the PRUSS memory region >>>> + * @size: size of the PRUSS memory region >>>> + */ >>>> +struct pruss_mem_region { >>>> + void __iomem *va; >>>> + phys_addr_t pa; >>>> + size_t size; >>>> +}; >>>> + >>>> struct device_node; >>>> struct rproc; >>>> struct pruss; >>>> @@ -52,6 +74,10 @@ struct pruss; >>>> struct pruss *pruss_get(struct rproc *rproc); >>>> void pruss_put(struct pruss *pruss); >>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>> + struct pruss_mem_region *region); >>>> +int pruss_release_mem_region(struct pruss *pruss, >>>> + struct pruss_mem_region *region); >>>> #else >>>> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc >>>> *rproc) >>>> static inline void pruss_put(struct pruss *pruss) { } >>>> +static inline int pruss_request_mem_region(struct pruss *pruss, >>>> + enum pruss_mem mem_id, >>>> + struct pruss_mem_region *region) >>>> +{ >>>> + return -EOPNOTSUPP; >>>> +} >>>> + >>>> +static inline int pruss_release_mem_region(struct pruss *pruss, >>>> + struct pruss_mem_region *region) >>>> +{ >>>> + return -EOPNOTSUPP; >>>> +} >>>> + >>>> #endif /* CONFIG_TI_PRUSS */ >>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) >>> >>> cheers, >>> -roger >>
Hi, On 21/03/2023 07:23, Md Danish Anwar wrote: > Hi Andrew, Roger, > > On 20/03/23 21:48, Andrew Davis wrote: >> On 3/20/23 12:11 AM, Md Danish Anwar wrote: >>> Hi Roger, >>> >>> On 17/03/23 14:26, Roger Quadros wrote: >>>> Hi Andrew & Danish, >>>> >>>> >>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>> From: "Andrew F. Davis" <afd@ti.com> >>>>> >>>>> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >>>>> to the PRUSS platform driver to allow client drivers to acquire and release >>>>> the common memory resources present within a PRU-ICSS subsystem. This >>>>> allows the client drivers to directly manipulate the respective memories, >>>>> as per their design contract with the associated firmware. >>>>> >>>>> Co-developed-by: Suman Anna <s-anna@ti.com> >>>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>> --- >>>>> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >>>>> include/linux/pruss_driver.h | 27 +++-------- >>>>> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ >>>> >>>> >>>> We have these 2 header files and I think anything that deals with >>>> 'struct pruss' should go in include/linux/pruss_driver.h >>>> >>>> Anything that deals with pru_rproc (i.e. struct rproc) should go in >>>> include/linux/remoteproc/pruss.h >>>> >>>> Do you agree? >>>> >>> >>> I agree with you Roger but Andrew is the right person to comment here as he is >>> the author of this and several other patches. >>> >>> Hi Andrew, Can you please comment on this? >>> >> >> Original idea was a consumer driver (like "ICSSG Ethernet Driver" in your other >> series) could just >> >> #include <linux/remoteproc/pruss.h> >> >> and get everything they need, and nothing they do not. >> > > If we plan on continuing the original idea, then I think keeping the header > files as it is will be the best. Because if we move anything that deals with > 'struct pruss' to include/linux/pruss_driver.h and anything that deals with > pru_rproc (i.e. struct rproc) to include/linux/remoteproc/pruss.h, then the > consumer drivers will need to do, > > #include <linux/remoteproc/pruss.h> > #include <linux/pruss_driver.h> > > Roger, should I keep the header files arrangement as it is? > OK but can we please rename one of them to something else so they don't sound very similar. Maybe you could use Andrew's suggestion below. >> pruss_driver.h (which could be renamed pruss_internal.h) exists to allow >> comunication between the pruss core and the pru rproc driver which live >> in different subsystems. >> >> Andrew >> >>>>> 3 files changed, 121 insertions(+), 22 deletions(-) >>>>> >>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>> index a169aa1ed044..c8053c0d735f 100644 >>>>> --- a/drivers/soc/ti/pruss.c >>>>> +++ b/drivers/soc/ti/pruss.c >>>>> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >>>>> } >>>>> EXPORT_SYMBOL_GPL(pruss_put); >>>>> +/** >>>>> + * pruss_request_mem_region() - request a memory resource >>>>> + * @pruss: the pruss instance >>>>> + * @mem_id: the memory resource id >>>>> + * @region: pointer to memory region structure to be filled in >>>>> + * >>>>> + * This function allows a client driver to request a memory resource, >>>>> + * and if successful, will let the client driver own the particular >>>>> + * memory region until released using the pruss_release_mem_region() >>>>> + * API. >>>>> + * >>>>> + * Return: 0 if requested memory region is available (in such case pointer to >>>>> + * memory region is returned via @region), an error otherwise >>>>> + */ >>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>> + struct pruss_mem_region *region) >>>>> +{ >>>>> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >>>>> + return -EINVAL; >>>>> + >>>>> + mutex_lock(&pruss->lock); >>>>> + >>>>> + if (pruss->mem_in_use[mem_id]) { >>>>> + mutex_unlock(&pruss->lock); >>>>> + return -EBUSY; >>>>> + } >>>>> + >>>>> + *region = pruss->mem_regions[mem_id]; >>>>> + pruss->mem_in_use[mem_id] = region; >>>>> + >>>>> + mutex_unlock(&pruss->lock); >>>>> + >>>>> + return 0; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >>>>> + >>>>> +/** >>>>> + * pruss_release_mem_region() - release a memory resource >>>>> + * @pruss: the pruss instance >>>>> + * @region: the memory region to release >>>>> + * >>>>> + * This function is the complimentary function to >>>>> + * pruss_request_mem_region(), and allows the client drivers to >>>>> + * release back a memory resource. >>>>> + * >>>>> + * Return: 0 on success, an error code otherwise >>>>> + */ >>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>> + struct pruss_mem_region *region) >>>>> +{ >>>>> + int id; >>>>> + >>>>> + if (!pruss || !region) >>>>> + return -EINVAL; >>>>> + >>>>> + mutex_lock(&pruss->lock); >>>>> + >>>>> + /* find out the memory region being released */ >>>>> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >>>>> + if (pruss->mem_in_use[id] == region) >>>>> + break; >>>>> + } >>>>> + >>>>> + if (id == PRUSS_MEM_MAX) { >>>>> + mutex_unlock(&pruss->lock); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + pruss->mem_in_use[id] = NULL; >>>>> + >>>>> + mutex_unlock(&pruss->lock); >>>>> + >>>>> + return 0; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >>>>> + >>>>> static void pruss_of_free_clk_provider(void *data) >>>>> { >>>>> struct device_node *clk_mux_np = data; >>>>> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >>>>> return -ENOMEM; >>>>> pruss->dev = dev; >>>>> + mutex_init(&pruss->lock); >>>>> child = of_get_child_by_name(np, "memories"); >>>>> if (!child) { >>>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >>>>> index 86242fb5a64a..22b4b37d2536 100644 >>>>> --- a/include/linux/pruss_driver.h >>>>> +++ b/include/linux/pruss_driver.h >>>>> @@ -9,37 +9,18 @@ >>>>> #ifndef _PRUSS_DRIVER_H_ >>>>> #define _PRUSS_DRIVER_H_ >>>>> +#include <linux/mutex.h> >>>>> #include <linux/remoteproc/pruss.h> >>>>> #include <linux/types.h> >>>>> -/* >>>>> - * enum pruss_mem - PRUSS memory range identifiers >>>>> - */ >>>>> -enum pruss_mem { >>>>> - PRUSS_MEM_DRAM0 = 0, >>>>> - PRUSS_MEM_DRAM1, >>>>> - PRUSS_MEM_SHRD_RAM2, >>>>> - PRUSS_MEM_MAX, >>>>> -}; >>>>> - >>>>> -/** >>>>> - * struct pruss_mem_region - PRUSS memory region structure >>>>> - * @va: kernel virtual address of the PRUSS memory region >>>>> - * @pa: physical (bus) address of the PRUSS memory region >>>>> - * @size: size of the PRUSS memory region >>>>> - */ >>>>> -struct pruss_mem_region { >>>>> - void __iomem *va; >>>>> - phys_addr_t pa; >>>>> - size_t size; >>>>> -}; >>>>> - >>>>> /** >>>>> * struct pruss - PRUSS parent structure >>>>> * @dev: pruss device pointer >>>>> * @cfg_base: base iomap for CFG region >>>>> * @cfg_regmap: regmap for config region >>>>> * @mem_regions: data for each of the PRUSS memory regions >>>>> + * @mem_in_use: to indicate if memory resource is in use >>>>> + * @lock: mutex to serialize access to resources >>>>> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >>>>> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >>>>> */ >>>>> @@ -48,6 +29,8 @@ struct pruss { >>>>> void __iomem *cfg_base; >>>>> struct regmap *cfg_regmap; >>>>> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >>>>> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >>>>> + struct mutex lock; /* PRU resource lock */ >>>>> struct clk *core_clk_mux; >>>>> struct clk *iep_clk_mux; >>>>> }; >>>>> diff --git a/include/linux/remoteproc/pruss.h >>>>> b/include/linux/remoteproc/pruss.h >>>>> index 93a98cac7829..33f930e0a0ce 100644 >>>>> --- a/include/linux/remoteproc/pruss.h >>>>> +++ b/include/linux/remoteproc/pruss.h >>>>> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >>>>> PRU_C31, >>>>> }; >>>>> +/* >>>>> + * enum pruss_mem - PRUSS memory range identifiers >>>>> + */ >>>>> +enum pruss_mem { >>>>> + PRUSS_MEM_DRAM0 = 0, >>>>> + PRUSS_MEM_DRAM1, >>>>> + PRUSS_MEM_SHRD_RAM2, >>>>> + PRUSS_MEM_MAX, >>>>> +}; >>>>> + >>>>> +/** >>>>> + * struct pruss_mem_region - PRUSS memory region structure >>>>> + * @va: kernel virtual address of the PRUSS memory region >>>>> + * @pa: physical (bus) address of the PRUSS memory region >>>>> + * @size: size of the PRUSS memory region >>>>> + */ >>>>> +struct pruss_mem_region { >>>>> + void __iomem *va; >>>>> + phys_addr_t pa; >>>>> + size_t size; >>>>> +}; >>>>> + >>>>> struct device_node; >>>>> struct rproc; >>>>> struct pruss; >>>>> @@ -52,6 +74,10 @@ struct pruss; >>>>> struct pruss *pruss_get(struct rproc *rproc); >>>>> void pruss_put(struct pruss *pruss); >>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>> + struct pruss_mem_region *region); >>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>> + struct pruss_mem_region *region); >>>>> #else >>>>> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc >>>>> *rproc) >>>>> static inline void pruss_put(struct pruss *pruss) { } >>>>> +static inline int pruss_request_mem_region(struct pruss *pruss, >>>>> + enum pruss_mem mem_id, >>>>> + struct pruss_mem_region *region) >>>>> +{ >>>>> + return -EOPNOTSUPP; >>>>> +} >>>>> + >>>>> +static inline int pruss_release_mem_region(struct pruss *pruss, >>>>> + struct pruss_mem_region *region) >>>>> +{ >>>>> + return -EOPNOTSUPP; >>>>> +} >>>>> + >>>>> #endif /* CONFIG_TI_PRUSS */ >>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) >>>> >>>> cheers, >>>> -roger >>> > cheers, -roger
Hi Roger, On 21/03/23 14:54, Roger Quadros wrote: > Hi, > > On 21/03/2023 07:23, Md Danish Anwar wrote: >> Hi Andrew, Roger, >> >> On 20/03/23 21:48, Andrew Davis wrote: >>> On 3/20/23 12:11 AM, Md Danish Anwar wrote: >>>> Hi Roger, >>>> >>>> On 17/03/23 14:26, Roger Quadros wrote: >>>>> Hi Andrew & Danish, >>>>> >>>>> >>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>> From: "Andrew F. Davis" <afd@ti.com> >>>>>> >>>>>> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >>>>>> to the PRUSS platform driver to allow client drivers to acquire and release >>>>>> the common memory resources present within a PRU-ICSS subsystem. This >>>>>> allows the client drivers to directly manipulate the respective memories, >>>>>> as per their design contract with the associated firmware. >>>>>> >>>>>> Co-developed-by: Suman Anna <s-anna@ti.com> >>>>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>>> --- >>>>>> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >>>>>> include/linux/pruss_driver.h | 27 +++-------- >>>>>> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ >>>>> >>>>> >>>>> We have these 2 header files and I think anything that deals with >>>>> 'struct pruss' should go in include/linux/pruss_driver.h >>>>> >>>>> Anything that deals with pru_rproc (i.e. struct rproc) should go in >>>>> include/linux/remoteproc/pruss.h >>>>> >>>>> Do you agree? >>>>> >>>> >>>> I agree with you Roger but Andrew is the right person to comment here as he is >>>> the author of this and several other patches. >>>> >>>> Hi Andrew, Can you please comment on this? >>>> >>> >>> Original idea was a consumer driver (like "ICSSG Ethernet Driver" in your other >>> series) could just >>> >>> #include <linux/remoteproc/pruss.h> >>> >>> and get everything they need, and nothing they do not. >>> >> >> If we plan on continuing the original idea, then I think keeping the header >> files as it is will be the best. Because if we move anything that deals with >> 'struct pruss' to include/linux/pruss_driver.h and anything that deals with >> pru_rproc (i.e. struct rproc) to include/linux/remoteproc/pruss.h, then the >> consumer drivers will need to do, >> >> #include <linux/remoteproc/pruss.h> >> #include <linux/pruss_driver.h> >> >> Roger, should I keep the header files arrangement as it is? >> > > OK but can we please rename one of them to something else so they don't > sound very similar. Maybe you could use Andrew's suggestion below. > Yes sure, I'll rename the header files to reduce confusion. The pruss_driver.h is located in include/linux, implying it's not internal to PRUSS. So I will keep this header file name as it is. There are total 3 pruss related header files. 1. include/linux/pruss_driver.h (Public header file, not internal to PRUSS, will keep it as it is. This exists to allow communication between the pruss core and the pru rproc driver which live in different subsystems.) 2. include/linux/remoteproc/pruss.h (Public header file, not internal to PRUSS, will keep it as it is. Only this header file needs to be included by client drivers.) 3. drivers/soc/ti/pruss.h (Internal to PRUSS, I will rename this to pruss_internal.h, this file has private definitions and APIs to modify PRUSS CFG space. This file is private to pruss.c) Please let me know if the above looks OK. >>> pruss_driver.h (which could be renamed pruss_internal.h) exists to allow >>> comunication between the pruss core and the pru rproc driver which live >>> in different subsystems. >>> >>> Andrew >>> >>>>>> 3 files changed, 121 insertions(+), 22 deletions(-) >>>>>> >>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>>> index a169aa1ed044..c8053c0d735f 100644 >>>>>> --- a/drivers/soc/ti/pruss.c >>>>>> +++ b/drivers/soc/ti/pruss.c >>>>>> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(pruss_put); >>>>>> +/** >>>>>> + * pruss_request_mem_region() - request a memory resource >>>>>> + * @pruss: the pruss instance >>>>>> + * @mem_id: the memory resource id >>>>>> + * @region: pointer to memory region structure to be filled in >>>>>> + * >>>>>> + * This function allows a client driver to request a memory resource, >>>>>> + * and if successful, will let the client driver own the particular >>>>>> + * memory region until released using the pruss_release_mem_region() >>>>>> + * API. >>>>>> + * >>>>>> + * Return: 0 if requested memory region is available (in such case pointer to >>>>>> + * memory region is returned via @region), an error otherwise >>>>>> + */ >>>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>>> + struct pruss_mem_region *region) >>>>>> +{ >>>>>> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + mutex_lock(&pruss->lock); >>>>>> + >>>>>> + if (pruss->mem_in_use[mem_id]) { >>>>>> + mutex_unlock(&pruss->lock); >>>>>> + return -EBUSY; >>>>>> + } >>>>>> + >>>>>> + *region = pruss->mem_regions[mem_id]; >>>>>> + pruss->mem_in_use[mem_id] = region; >>>>>> + >>>>>> + mutex_unlock(&pruss->lock); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >>>>>> + >>>>>> +/** >>>>>> + * pruss_release_mem_region() - release a memory resource >>>>>> + * @pruss: the pruss instance >>>>>> + * @region: the memory region to release >>>>>> + * >>>>>> + * This function is the complimentary function to >>>>>> + * pruss_request_mem_region(), and allows the client drivers to >>>>>> + * release back a memory resource. >>>>>> + * >>>>>> + * Return: 0 on success, an error code otherwise >>>>>> + */ >>>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>>> + struct pruss_mem_region *region) >>>>>> +{ >>>>>> + int id; >>>>>> + >>>>>> + if (!pruss || !region) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + mutex_lock(&pruss->lock); >>>>>> + >>>>>> + /* find out the memory region being released */ >>>>>> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >>>>>> + if (pruss->mem_in_use[id] == region) >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + if (id == PRUSS_MEM_MAX) { >>>>>> + mutex_unlock(&pruss->lock); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + pruss->mem_in_use[id] = NULL; >>>>>> + >>>>>> + mutex_unlock(&pruss->lock); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >>>>>> + >>>>>> static void pruss_of_free_clk_provider(void *data) >>>>>> { >>>>>> struct device_node *clk_mux_np = data; >>>>>> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >>>>>> return -ENOMEM; >>>>>> pruss->dev = dev; >>>>>> + mutex_init(&pruss->lock); >>>>>> child = of_get_child_by_name(np, "memories"); >>>>>> if (!child) { >>>>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >>>>>> index 86242fb5a64a..22b4b37d2536 100644 >>>>>> --- a/include/linux/pruss_driver.h >>>>>> +++ b/include/linux/pruss_driver.h >>>>>> @@ -9,37 +9,18 @@ >>>>>> #ifndef _PRUSS_DRIVER_H_ >>>>>> #define _PRUSS_DRIVER_H_ >>>>>> +#include <linux/mutex.h> >>>>>> #include <linux/remoteproc/pruss.h> >>>>>> #include <linux/types.h> >>>>>> -/* >>>>>> - * enum pruss_mem - PRUSS memory range identifiers >>>>>> - */ >>>>>> -enum pruss_mem { >>>>>> - PRUSS_MEM_DRAM0 = 0, >>>>>> - PRUSS_MEM_DRAM1, >>>>>> - PRUSS_MEM_SHRD_RAM2, >>>>>> - PRUSS_MEM_MAX, >>>>>> -}; >>>>>> - >>>>>> -/** >>>>>> - * struct pruss_mem_region - PRUSS memory region structure >>>>>> - * @va: kernel virtual address of the PRUSS memory region >>>>>> - * @pa: physical (bus) address of the PRUSS memory region >>>>>> - * @size: size of the PRUSS memory region >>>>>> - */ >>>>>> -struct pruss_mem_region { >>>>>> - void __iomem *va; >>>>>> - phys_addr_t pa; >>>>>> - size_t size; >>>>>> -}; >>>>>> - >>>>>> /** >>>>>> * struct pruss - PRUSS parent structure >>>>>> * @dev: pruss device pointer >>>>>> * @cfg_base: base iomap for CFG region >>>>>> * @cfg_regmap: regmap for config region >>>>>> * @mem_regions: data for each of the PRUSS memory regions >>>>>> + * @mem_in_use: to indicate if memory resource is in use >>>>>> + * @lock: mutex to serialize access to resources >>>>>> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >>>>>> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >>>>>> */ >>>>>> @@ -48,6 +29,8 @@ struct pruss { >>>>>> void __iomem *cfg_base; >>>>>> struct regmap *cfg_regmap; >>>>>> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >>>>>> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >>>>>> + struct mutex lock; /* PRU resource lock */ >>>>>> struct clk *core_clk_mux; >>>>>> struct clk *iep_clk_mux; >>>>>> }; >>>>>> diff --git a/include/linux/remoteproc/pruss.h >>>>>> b/include/linux/remoteproc/pruss.h >>>>>> index 93a98cac7829..33f930e0a0ce 100644 >>>>>> --- a/include/linux/remoteproc/pruss.h >>>>>> +++ b/include/linux/remoteproc/pruss.h >>>>>> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >>>>>> PRU_C31, >>>>>> }; >>>>>> +/* >>>>>> + * enum pruss_mem - PRUSS memory range identifiers >>>>>> + */ >>>>>> +enum pruss_mem { >>>>>> + PRUSS_MEM_DRAM0 = 0, >>>>>> + PRUSS_MEM_DRAM1, >>>>>> + PRUSS_MEM_SHRD_RAM2, >>>>>> + PRUSS_MEM_MAX, >>>>>> +}; >>>>>> + >>>>>> +/** >>>>>> + * struct pruss_mem_region - PRUSS memory region structure >>>>>> + * @va: kernel virtual address of the PRUSS memory region >>>>>> + * @pa: physical (bus) address of the PRUSS memory region >>>>>> + * @size: size of the PRUSS memory region >>>>>> + */ >>>>>> +struct pruss_mem_region { >>>>>> + void __iomem *va; >>>>>> + phys_addr_t pa; >>>>>> + size_t size; >>>>>> +}; >>>>>> + >>>>>> struct device_node; >>>>>> struct rproc; >>>>>> struct pruss; >>>>>> @@ -52,6 +74,10 @@ struct pruss; >>>>>> struct pruss *pruss_get(struct rproc *rproc); >>>>>> void pruss_put(struct pruss *pruss); >>>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>>> + struct pruss_mem_region *region); >>>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>>> + struct pruss_mem_region *region); >>>>>> #else >>>>>> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc >>>>>> *rproc) >>>>>> static inline void pruss_put(struct pruss *pruss) { } >>>>>> +static inline int pruss_request_mem_region(struct pruss *pruss, >>>>>> + enum pruss_mem mem_id, >>>>>> + struct pruss_mem_region *region) >>>>>> +{ >>>>>> + return -EOPNOTSUPP; >>>>>> +} >>>>>> + >>>>>> +static inline int pruss_release_mem_region(struct pruss *pruss, >>>>>> + struct pruss_mem_region *region) >>>>>> +{ >>>>>> + return -EOPNOTSUPP; >>>>>> +} >>>>>> + >>>>>> #endif /* CONFIG_TI_PRUSS */ >>>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) >>>>> >>>>> cheers, >>>>> -roger >>>> >> > > cheers, > -roger
On 21/03/2023 11:48, Md Danish Anwar wrote: > Hi Roger, > > On 21/03/23 14:54, Roger Quadros wrote: >> Hi, >> >> On 21/03/2023 07:23, Md Danish Anwar wrote: >>> Hi Andrew, Roger, >>> >>> On 20/03/23 21:48, Andrew Davis wrote: >>>> On 3/20/23 12:11 AM, Md Danish Anwar wrote: >>>>> Hi Roger, >>>>> >>>>> On 17/03/23 14:26, Roger Quadros wrote: >>>>>> Hi Andrew & Danish, >>>>>> >>>>>> >>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>>> From: "Andrew F. Davis" <afd@ti.com> >>>>>>> >>>>>>> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >>>>>>> to the PRUSS platform driver to allow client drivers to acquire and release >>>>>>> the common memory resources present within a PRU-ICSS subsystem. This >>>>>>> allows the client drivers to directly manipulate the respective memories, >>>>>>> as per their design contract with the associated firmware. >>>>>>> >>>>>>> Co-developed-by: Suman Anna <s-anna@ti.com> >>>>>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>>>> --- >>>>>>> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >>>>>>> include/linux/pruss_driver.h | 27 +++-------- >>>>>>> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ >>>>>> >>>>>> >>>>>> We have these 2 header files and I think anything that deals with >>>>>> 'struct pruss' should go in include/linux/pruss_driver.h >>>>>> >>>>>> Anything that deals with pru_rproc (i.e. struct rproc) should go in >>>>>> include/linux/remoteproc/pruss.h >>>>>> >>>>>> Do you agree? >>>>>> >>>>> >>>>> I agree with you Roger but Andrew is the right person to comment here as he is >>>>> the author of this and several other patches. >>>>> >>>>> Hi Andrew, Can you please comment on this? >>>>> >>>> >>>> Original idea was a consumer driver (like "ICSSG Ethernet Driver" in your other >>>> series) could just >>>> >>>> #include <linux/remoteproc/pruss.h> >>>> >>>> and get everything they need, and nothing they do not. >>>> >>> >>> If we plan on continuing the original idea, then I think keeping the header >>> files as it is will be the best. Because if we move anything that deals with >>> 'struct pruss' to include/linux/pruss_driver.h and anything that deals with >>> pru_rproc (i.e. struct rproc) to include/linux/remoteproc/pruss.h, then the >>> consumer drivers will need to do, >>> >>> #include <linux/remoteproc/pruss.h> >>> #include <linux/pruss_driver.h> >>> >>> Roger, should I keep the header files arrangement as it is? >>> >> >> OK but can we please rename one of them to something else so they don't >> sound very similar. Maybe you could use Andrew's suggestion below. >> > > Yes sure, I'll rename the header files to reduce confusion. The pruss_driver.h > is located in include/linux, implying it's not internal to PRUSS. So I will > keep this header file name as it is. > > There are total 3 pruss related header files. > > 1. include/linux/pruss_driver.h (Public header file, not internal to PRUSS, > will keep it as it is. This exists to allow communication between the pruss > core and the pru rproc driver which live in different subsystems.) Andrew asked you to rename this to pruss_internal.h > 2. include/linux/remoteproc/pruss.h (Public header file, not internal to PRUSS, > will keep it as it is. Only this header file needs to be included by client > drivers.) > 3. drivers/soc/ti/pruss.h (Internal to PRUSS, I will rename this to > pruss_internal.h, this file has private definitions and APIs to modify PRUSS > CFG space. This file is private to pruss.c) No point in changing this file's name as this is not visible elsewhere. > > Please let me know if the above looks OK. > >>>> pruss_driver.h (which could be renamed pruss_internal.h) exists to allow >>>> comunication between the pruss core and the pru rproc driver which live >>>> in different subsystems. >>>> >>>> Andrew >>>> >>>>>>> 3 files changed, 121 insertions(+), 22 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>>>> index a169aa1ed044..c8053c0d735f 100644 >>>>>>> --- a/drivers/soc/ti/pruss.c >>>>>>> +++ b/drivers/soc/ti/pruss.c >>>>>>> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >>>>>>> } >>>>>>> EXPORT_SYMBOL_GPL(pruss_put); >>>>>>> +/** >>>>>>> + * pruss_request_mem_region() - request a memory resource >>>>>>> + * @pruss: the pruss instance >>>>>>> + * @mem_id: the memory resource id >>>>>>> + * @region: pointer to memory region structure to be filled in >>>>>>> + * >>>>>>> + * This function allows a client driver to request a memory resource, >>>>>>> + * and if successful, will let the client driver own the particular >>>>>>> + * memory region until released using the pruss_release_mem_region() >>>>>>> + * API. >>>>>>> + * >>>>>>> + * Return: 0 if requested memory region is available (in such case pointer to >>>>>>> + * memory region is returned via @region), an error otherwise >>>>>>> + */ >>>>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>>>> + struct pruss_mem_region *region) >>>>>>> +{ >>>>>>> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + mutex_lock(&pruss->lock); >>>>>>> + >>>>>>> + if (pruss->mem_in_use[mem_id]) { >>>>>>> + mutex_unlock(&pruss->lock); >>>>>>> + return -EBUSY; >>>>>>> + } >>>>>>> + >>>>>>> + *region = pruss->mem_regions[mem_id]; >>>>>>> + pruss->mem_in_use[mem_id] = region; >>>>>>> + >>>>>>> + mutex_unlock(&pruss->lock); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >>>>>>> + >>>>>>> +/** >>>>>>> + * pruss_release_mem_region() - release a memory resource >>>>>>> + * @pruss: the pruss instance >>>>>>> + * @region: the memory region to release >>>>>>> + * >>>>>>> + * This function is the complimentary function to >>>>>>> + * pruss_request_mem_region(), and allows the client drivers to >>>>>>> + * release back a memory resource. >>>>>>> + * >>>>>>> + * Return: 0 on success, an error code otherwise >>>>>>> + */ >>>>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>>>> + struct pruss_mem_region *region) >>>>>>> +{ >>>>>>> + int id; >>>>>>> + >>>>>>> + if (!pruss || !region) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + mutex_lock(&pruss->lock); >>>>>>> + >>>>>>> + /* find out the memory region being released */ >>>>>>> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >>>>>>> + if (pruss->mem_in_use[id] == region) >>>>>>> + break; >>>>>>> + } >>>>>>> + >>>>>>> + if (id == PRUSS_MEM_MAX) { >>>>>>> + mutex_unlock(&pruss->lock); >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>>> + >>>>>>> + pruss->mem_in_use[id] = NULL; >>>>>>> + >>>>>>> + mutex_unlock(&pruss->lock); >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >>>>>>> + >>>>>>> static void pruss_of_free_clk_provider(void *data) >>>>>>> { >>>>>>> struct device_node *clk_mux_np = data; >>>>>>> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >>>>>>> return -ENOMEM; >>>>>>> pruss->dev = dev; >>>>>>> + mutex_init(&pruss->lock); >>>>>>> child = of_get_child_by_name(np, "memories"); >>>>>>> if (!child) { >>>>>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >>>>>>> index 86242fb5a64a..22b4b37d2536 100644 >>>>>>> --- a/include/linux/pruss_driver.h >>>>>>> +++ b/include/linux/pruss_driver.h >>>>>>> @@ -9,37 +9,18 @@ >>>>>>> #ifndef _PRUSS_DRIVER_H_ >>>>>>> #define _PRUSS_DRIVER_H_ >>>>>>> +#include <linux/mutex.h> >>>>>>> #include <linux/remoteproc/pruss.h> >>>>>>> #include <linux/types.h> >>>>>>> -/* >>>>>>> - * enum pruss_mem - PRUSS memory range identifiers >>>>>>> - */ >>>>>>> -enum pruss_mem { >>>>>>> - PRUSS_MEM_DRAM0 = 0, >>>>>>> - PRUSS_MEM_DRAM1, >>>>>>> - PRUSS_MEM_SHRD_RAM2, >>>>>>> - PRUSS_MEM_MAX, >>>>>>> -}; >>>>>>> - >>>>>>> -/** >>>>>>> - * struct pruss_mem_region - PRUSS memory region structure >>>>>>> - * @va: kernel virtual address of the PRUSS memory region >>>>>>> - * @pa: physical (bus) address of the PRUSS memory region >>>>>>> - * @size: size of the PRUSS memory region >>>>>>> - */ >>>>>>> -struct pruss_mem_region { >>>>>>> - void __iomem *va; >>>>>>> - phys_addr_t pa; >>>>>>> - size_t size; >>>>>>> -}; >>>>>>> - >>>>>>> /** >>>>>>> * struct pruss - PRUSS parent structure >>>>>>> * @dev: pruss device pointer >>>>>>> * @cfg_base: base iomap for CFG region >>>>>>> * @cfg_regmap: regmap for config region >>>>>>> * @mem_regions: data for each of the PRUSS memory regions >>>>>>> + * @mem_in_use: to indicate if memory resource is in use >>>>>>> + * @lock: mutex to serialize access to resources >>>>>>> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >>>>>>> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >>>>>>> */ >>>>>>> @@ -48,6 +29,8 @@ struct pruss { >>>>>>> void __iomem *cfg_base; >>>>>>> struct regmap *cfg_regmap; >>>>>>> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >>>>>>> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >>>>>>> + struct mutex lock; /* PRU resource lock */ >>>>>>> struct clk *core_clk_mux; >>>>>>> struct clk *iep_clk_mux; >>>>>>> }; >>>>>>> diff --git a/include/linux/remoteproc/pruss.h >>>>>>> b/include/linux/remoteproc/pruss.h >>>>>>> index 93a98cac7829..33f930e0a0ce 100644 >>>>>>> --- a/include/linux/remoteproc/pruss.h >>>>>>> +++ b/include/linux/remoteproc/pruss.h >>>>>>> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >>>>>>> PRU_C31, >>>>>>> }; >>>>>>> +/* >>>>>>> + * enum pruss_mem - PRUSS memory range identifiers >>>>>>> + */ >>>>>>> +enum pruss_mem { >>>>>>> + PRUSS_MEM_DRAM0 = 0, >>>>>>> + PRUSS_MEM_DRAM1, >>>>>>> + PRUSS_MEM_SHRD_RAM2, >>>>>>> + PRUSS_MEM_MAX, >>>>>>> +}; >>>>>>> + >>>>>>> +/** >>>>>>> + * struct pruss_mem_region - PRUSS memory region structure >>>>>>> + * @va: kernel virtual address of the PRUSS memory region >>>>>>> + * @pa: physical (bus) address of the PRUSS memory region >>>>>>> + * @size: size of the PRUSS memory region >>>>>>> + */ >>>>>>> +struct pruss_mem_region { >>>>>>> + void __iomem *va; >>>>>>> + phys_addr_t pa; >>>>>>> + size_t size; >>>>>>> +}; >>>>>>> + >>>>>>> struct device_node; >>>>>>> struct rproc; >>>>>>> struct pruss; >>>>>>> @@ -52,6 +74,10 @@ struct pruss; >>>>>>> struct pruss *pruss_get(struct rproc *rproc); >>>>>>> void pruss_put(struct pruss *pruss); >>>>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>>>> + struct pruss_mem_region *region); >>>>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>>>> + struct pruss_mem_region *region); >>>>>>> #else >>>>>>> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc >>>>>>> *rproc) >>>>>>> static inline void pruss_put(struct pruss *pruss) { } >>>>>>> +static inline int pruss_request_mem_region(struct pruss *pruss, >>>>>>> + enum pruss_mem mem_id, >>>>>>> + struct pruss_mem_region *region) >>>>>>> +{ >>>>>>> + return -EOPNOTSUPP; >>>>>>> +} >>>>>>> + >>>>>>> +static inline int pruss_release_mem_region(struct pruss *pruss, >>>>>>> + struct pruss_mem_region *region) >>>>>>> +{ >>>>>>> + return -EOPNOTSUPP; >>>>>>> +} >>>>>>> + >>>>>>> #endif /* CONFIG_TI_PRUSS */ >>>>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) >>>>>> cheers, -roger
On 21/03/23 15:53, Roger Quadros wrote: > > > On 21/03/2023 11:48, Md Danish Anwar wrote: >> Hi Roger, >> >> On 21/03/23 14:54, Roger Quadros wrote: >>> Hi, >>> >>> On 21/03/2023 07:23, Md Danish Anwar wrote: >>>> Hi Andrew, Roger, >>>> >>>> On 20/03/23 21:48, Andrew Davis wrote: >>>>> On 3/20/23 12:11 AM, Md Danish Anwar wrote: >>>>>> Hi Roger, >>>>>> >>>>>> On 17/03/23 14:26, Roger Quadros wrote: >>>>>>> Hi Andrew & Danish, >>>>>>> >>>>>>> >>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>>>> From: "Andrew F. Davis" <afd@ti.com> >>>>>>>> >>>>>>>> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(), >>>>>>>> to the PRUSS platform driver to allow client drivers to acquire and release >>>>>>>> the common memory resources present within a PRU-ICSS subsystem. This >>>>>>>> allows the client drivers to directly manipulate the respective memories, >>>>>>>> as per their design contract with the associated firmware. >>>>>>>> >>>>>>>> Co-developed-by: Suman Anna <s-anna@ti.com> >>>>>>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org> >>>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com> >>>>>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org> >>>>>>>> --- >>>>>>>> drivers/soc/ti/pruss.c | 77 ++++++++++++++++++++++++++++++++ >>>>>>>> include/linux/pruss_driver.h | 27 +++-------- >>>>>>>> include/linux/remoteproc/pruss.h | 39 ++++++++++++++++ >>>>>>> >>>>>>> >>>>>>> We have these 2 header files and I think anything that deals with >>>>>>> 'struct pruss' should go in include/linux/pruss_driver.h >>>>>>> >>>>>>> Anything that deals with pru_rproc (i.e. struct rproc) should go in >>>>>>> include/linux/remoteproc/pruss.h >>>>>>> >>>>>>> Do you agree? >>>>>>> >>>>>> >>>>>> I agree with you Roger but Andrew is the right person to comment here as he is >>>>>> the author of this and several other patches. >>>>>> >>>>>> Hi Andrew, Can you please comment on this? >>>>>> >>>>> >>>>> Original idea was a consumer driver (like "ICSSG Ethernet Driver" in your other >>>>> series) could just >>>>> >>>>> #include <linux/remoteproc/pruss.h> >>>>> >>>>> and get everything they need, and nothing they do not. >>>>> >>>> >>>> If we plan on continuing the original idea, then I think keeping the header >>>> files as it is will be the best. Because if we move anything that deals with >>>> 'struct pruss' to include/linux/pruss_driver.h and anything that deals with >>>> pru_rproc (i.e. struct rproc) to include/linux/remoteproc/pruss.h, then the >>>> consumer drivers will need to do, >>>> >>>> #include <linux/remoteproc/pruss.h> >>>> #include <linux/pruss_driver.h> >>>> >>>> Roger, should I keep the header files arrangement as it is? >>>> >>> >>> OK but can we please rename one of them to something else so they don't >>> sound very similar. Maybe you could use Andrew's suggestion below. >>> >> >> Yes sure, I'll rename the header files to reduce confusion. The pruss_driver.h >> is located in include/linux, implying it's not internal to PRUSS. So I will >> keep this header file name as it is. >> >> There are total 3 pruss related header files. >> >> 1. include/linux/pruss_driver.h (Public header file, not internal to PRUSS, >> will keep it as it is. This exists to allow communication between the pruss >> core and the pru rproc driver which live in different subsystems.) > > Andrew asked you to rename this to pruss_internal.h OK, I will change pruss_driver.h to pruss_internal.h > >> 2. include/linux/remoteproc/pruss.h (Public header file, not internal to PRUSS, >> will keep it as it is. Only this header file needs to be included by client >> drivers.) >> 3. drivers/soc/ti/pruss.h (Internal to PRUSS, I will rename this to >> pruss_internal.h, this file has private definitions and APIs to modify PRUSS >> CFG space. This file is private to pruss.c) > > No point in changing this file's name as this is not visible elsewhere. > OK, I will keep drivers/soc/ti/pruss.h and include/linux/remoteproc/pruss.h as it is. >> >> Please let me know if the above looks OK. >> >>>>> pruss_driver.h (which could be renamed pruss_internal.h) exists to allow >>>>> comunication between the pruss core and the pru rproc driver which live >>>>> in different subsystems. >>>>> >>>>> Andrew >>>>> >>>>>>>> 3 files changed, 121 insertions(+), 22 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>>>>> index a169aa1ed044..c8053c0d735f 100644 >>>>>>>> --- a/drivers/soc/ti/pruss.c >>>>>>>> +++ b/drivers/soc/ti/pruss.c >>>>>>>> @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) >>>>>>>> } >>>>>>>> EXPORT_SYMBOL_GPL(pruss_put); >>>>>>>> +/** >>>>>>>> + * pruss_request_mem_region() - request a memory resource >>>>>>>> + * @pruss: the pruss instance >>>>>>>> + * @mem_id: the memory resource id >>>>>>>> + * @region: pointer to memory region structure to be filled in >>>>>>>> + * >>>>>>>> + * This function allows a client driver to request a memory resource, >>>>>>>> + * and if successful, will let the client driver own the particular >>>>>>>> + * memory region until released using the pruss_release_mem_region() >>>>>>>> + * API. >>>>>>>> + * >>>>>>>> + * Return: 0 if requested memory region is available (in such case pointer to >>>>>>>> + * memory region is returned via @region), an error otherwise >>>>>>>> + */ >>>>>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>>>>> + struct pruss_mem_region *region) >>>>>>>> +{ >>>>>>>> + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + mutex_lock(&pruss->lock); >>>>>>>> + >>>>>>>> + if (pruss->mem_in_use[mem_id]) { >>>>>>>> + mutex_unlock(&pruss->lock); >>>>>>>> + return -EBUSY; >>>>>>>> + } >>>>>>>> + >>>>>>>> + *region = pruss->mem_regions[mem_id]; >>>>>>>> + pruss->mem_in_use[mem_id] = region; >>>>>>>> + >>>>>>>> + mutex_unlock(&pruss->lock); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL_GPL(pruss_request_mem_region); >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * pruss_release_mem_region() - release a memory resource >>>>>>>> + * @pruss: the pruss instance >>>>>>>> + * @region: the memory region to release >>>>>>>> + * >>>>>>>> + * This function is the complimentary function to >>>>>>>> + * pruss_request_mem_region(), and allows the client drivers to >>>>>>>> + * release back a memory resource. >>>>>>>> + * >>>>>>>> + * Return: 0 on success, an error code otherwise >>>>>>>> + */ >>>>>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>>>>> + struct pruss_mem_region *region) >>>>>>>> +{ >>>>>>>> + int id; >>>>>>>> + >>>>>>>> + if (!pruss || !region) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + mutex_lock(&pruss->lock); >>>>>>>> + >>>>>>>> + /* find out the memory region being released */ >>>>>>>> + for (id = 0; id < PRUSS_MEM_MAX; id++) { >>>>>>>> + if (pruss->mem_in_use[id] == region) >>>>>>>> + break; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (id == PRUSS_MEM_MAX) { >>>>>>>> + mutex_unlock(&pruss->lock); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> + pruss->mem_in_use[id] = NULL; >>>>>>>> + >>>>>>>> + mutex_unlock(&pruss->lock); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> +EXPORT_SYMBOL_GPL(pruss_release_mem_region); >>>>>>>> + >>>>>>>> static void pruss_of_free_clk_provider(void *data) >>>>>>>> { >>>>>>>> struct device_node *clk_mux_np = data; >>>>>>>> @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) >>>>>>>> return -ENOMEM; >>>>>>>> pruss->dev = dev; >>>>>>>> + mutex_init(&pruss->lock); >>>>>>>> child = of_get_child_by_name(np, "memories"); >>>>>>>> if (!child) { >>>>>>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h >>>>>>>> index 86242fb5a64a..22b4b37d2536 100644 >>>>>>>> --- a/include/linux/pruss_driver.h >>>>>>>> +++ b/include/linux/pruss_driver.h >>>>>>>> @@ -9,37 +9,18 @@ >>>>>>>> #ifndef _PRUSS_DRIVER_H_ >>>>>>>> #define _PRUSS_DRIVER_H_ >>>>>>>> +#include <linux/mutex.h> >>>>>>>> #include <linux/remoteproc/pruss.h> >>>>>>>> #include <linux/types.h> >>>>>>>> -/* >>>>>>>> - * enum pruss_mem - PRUSS memory range identifiers >>>>>>>> - */ >>>>>>>> -enum pruss_mem { >>>>>>>> - PRUSS_MEM_DRAM0 = 0, >>>>>>>> - PRUSS_MEM_DRAM1, >>>>>>>> - PRUSS_MEM_SHRD_RAM2, >>>>>>>> - PRUSS_MEM_MAX, >>>>>>>> -}; >>>>>>>> - >>>>>>>> -/** >>>>>>>> - * struct pruss_mem_region - PRUSS memory region structure >>>>>>>> - * @va: kernel virtual address of the PRUSS memory region >>>>>>>> - * @pa: physical (bus) address of the PRUSS memory region >>>>>>>> - * @size: size of the PRUSS memory region >>>>>>>> - */ >>>>>>>> -struct pruss_mem_region { >>>>>>>> - void __iomem *va; >>>>>>>> - phys_addr_t pa; >>>>>>>> - size_t size; >>>>>>>> -}; >>>>>>>> - >>>>>>>> /** >>>>>>>> * struct pruss - PRUSS parent structure >>>>>>>> * @dev: pruss device pointer >>>>>>>> * @cfg_base: base iomap for CFG region >>>>>>>> * @cfg_regmap: regmap for config region >>>>>>>> * @mem_regions: data for each of the PRUSS memory regions >>>>>>>> + * @mem_in_use: to indicate if memory resource is in use >>>>>>>> + * @lock: mutex to serialize access to resources >>>>>>>> * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX >>>>>>>> * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX >>>>>>>> */ >>>>>>>> @@ -48,6 +29,8 @@ struct pruss { >>>>>>>> void __iomem *cfg_base; >>>>>>>> struct regmap *cfg_regmap; >>>>>>>> struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; >>>>>>>> + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; >>>>>>>> + struct mutex lock; /* PRU resource lock */ >>>>>>>> struct clk *core_clk_mux; >>>>>>>> struct clk *iep_clk_mux; >>>>>>>> }; >>>>>>>> diff --git a/include/linux/remoteproc/pruss.h >>>>>>>> b/include/linux/remoteproc/pruss.h >>>>>>>> index 93a98cac7829..33f930e0a0ce 100644 >>>>>>>> --- a/include/linux/remoteproc/pruss.h >>>>>>>> +++ b/include/linux/remoteproc/pruss.h >>>>>>>> @@ -44,6 +44,28 @@ enum pru_ctable_idx { >>>>>>>> PRU_C31, >>>>>>>> }; >>>>>>>> +/* >>>>>>>> + * enum pruss_mem - PRUSS memory range identifiers >>>>>>>> + */ >>>>>>>> +enum pruss_mem { >>>>>>>> + PRUSS_MEM_DRAM0 = 0, >>>>>>>> + PRUSS_MEM_DRAM1, >>>>>>>> + PRUSS_MEM_SHRD_RAM2, >>>>>>>> + PRUSS_MEM_MAX, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +/** >>>>>>>> + * struct pruss_mem_region - PRUSS memory region structure >>>>>>>> + * @va: kernel virtual address of the PRUSS memory region >>>>>>>> + * @pa: physical (bus) address of the PRUSS memory region >>>>>>>> + * @size: size of the PRUSS memory region >>>>>>>> + */ >>>>>>>> +struct pruss_mem_region { >>>>>>>> + void __iomem *va; >>>>>>>> + phys_addr_t pa; >>>>>>>> + size_t size; >>>>>>>> +}; >>>>>>>> + >>>>>>>> struct device_node; >>>>>>>> struct rproc; >>>>>>>> struct pruss; >>>>>>>> @@ -52,6 +74,10 @@ struct pruss; >>>>>>>> struct pruss *pruss_get(struct rproc *rproc); >>>>>>>> void pruss_put(struct pruss *pruss); >>>>>>>> +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >>>>>>>> + struct pruss_mem_region *region); >>>>>>>> +int pruss_release_mem_region(struct pruss *pruss, >>>>>>>> + struct pruss_mem_region *region); >>>>>>>> #else >>>>>>>> @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc >>>>>>>> *rproc) >>>>>>>> static inline void pruss_put(struct pruss *pruss) { } >>>>>>>> +static inline int pruss_request_mem_region(struct pruss *pruss, >>>>>>>> + enum pruss_mem mem_id, >>>>>>>> + struct pruss_mem_region *region) >>>>>>>> +{ >>>>>>>> + return -EOPNOTSUPP; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static inline int pruss_release_mem_region(struct pruss *pruss, >>>>>>>> + struct pruss_mem_region *region) >>>>>>>> +{ >>>>>>>> + return -EOPNOTSUPP; >>>>>>>> +} >>>>>>>> + >>>>>>>> #endif /* CONFIG_TI_PRUSS */ >>>>>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) >>>>>>> > > > cheers, > -roger
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c index a169aa1ed044..c8053c0d735f 100644 --- a/drivers/soc/ti/pruss.c +++ b/drivers/soc/ti/pruss.c @@ -88,6 +88,82 @@ void pruss_put(struct pruss *pruss) } EXPORT_SYMBOL_GPL(pruss_put); +/** + * pruss_request_mem_region() - request a memory resource + * @pruss: the pruss instance + * @mem_id: the memory resource id + * @region: pointer to memory region structure to be filled in + * + * This function allows a client driver to request a memory resource, + * and if successful, will let the client driver own the particular + * memory region until released using the pruss_release_mem_region() + * API. + * + * Return: 0 if requested memory region is available (in such case pointer to + * memory region is returned via @region), an error otherwise + */ +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, + struct pruss_mem_region *region) +{ + if (!pruss || !region || mem_id >= PRUSS_MEM_MAX) + return -EINVAL; + + mutex_lock(&pruss->lock); + + if (pruss->mem_in_use[mem_id]) { + mutex_unlock(&pruss->lock); + return -EBUSY; + } + + *region = pruss->mem_regions[mem_id]; + pruss->mem_in_use[mem_id] = region; + + mutex_unlock(&pruss->lock); + + return 0; +} +EXPORT_SYMBOL_GPL(pruss_request_mem_region); + +/** + * pruss_release_mem_region() - release a memory resource + * @pruss: the pruss instance + * @region: the memory region to release + * + * This function is the complimentary function to + * pruss_request_mem_region(), and allows the client drivers to + * release back a memory resource. + * + * Return: 0 on success, an error code otherwise + */ +int pruss_release_mem_region(struct pruss *pruss, + struct pruss_mem_region *region) +{ + int id; + + if (!pruss || !region) + return -EINVAL; + + mutex_lock(&pruss->lock); + + /* find out the memory region being released */ + for (id = 0; id < PRUSS_MEM_MAX; id++) { + if (pruss->mem_in_use[id] == region) + break; + } + + if (id == PRUSS_MEM_MAX) { + mutex_unlock(&pruss->lock); + return -EINVAL; + } + + pruss->mem_in_use[id] = NULL; + + mutex_unlock(&pruss->lock); + + return 0; +} +EXPORT_SYMBOL_GPL(pruss_release_mem_region); + static void pruss_of_free_clk_provider(void *data) { struct device_node *clk_mux_np = data; @@ -290,6 +366,7 @@ static int pruss_probe(struct platform_device *pdev) return -ENOMEM; pruss->dev = dev; + mutex_init(&pruss->lock); child = of_get_child_by_name(np, "memories"); if (!child) { diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h index 86242fb5a64a..22b4b37d2536 100644 --- a/include/linux/pruss_driver.h +++ b/include/linux/pruss_driver.h @@ -9,37 +9,18 @@ #ifndef _PRUSS_DRIVER_H_ #define _PRUSS_DRIVER_H_ +#include <linux/mutex.h> #include <linux/remoteproc/pruss.h> #include <linux/types.h> -/* - * enum pruss_mem - PRUSS memory range identifiers - */ -enum pruss_mem { - PRUSS_MEM_DRAM0 = 0, - PRUSS_MEM_DRAM1, - PRUSS_MEM_SHRD_RAM2, - PRUSS_MEM_MAX, -}; - -/** - * struct pruss_mem_region - PRUSS memory region structure - * @va: kernel virtual address of the PRUSS memory region - * @pa: physical (bus) address of the PRUSS memory region - * @size: size of the PRUSS memory region - */ -struct pruss_mem_region { - void __iomem *va; - phys_addr_t pa; - size_t size; -}; - /** * struct pruss - PRUSS parent structure * @dev: pruss device pointer * @cfg_base: base iomap for CFG region * @cfg_regmap: regmap for config region * @mem_regions: data for each of the PRUSS memory regions + * @mem_in_use: to indicate if memory resource is in use + * @lock: mutex to serialize access to resources * @core_clk_mux: clk handle for PRUSS CORE_CLK_MUX * @iep_clk_mux: clk handle for PRUSS IEP_CLK_MUX */ @@ -48,6 +29,8 @@ struct pruss { void __iomem *cfg_base; struct regmap *cfg_regmap; struct pruss_mem_region mem_regions[PRUSS_MEM_MAX]; + struct pruss_mem_region *mem_in_use[PRUSS_MEM_MAX]; + struct mutex lock; /* PRU resource lock */ struct clk *core_clk_mux; struct clk *iep_clk_mux; }; diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h index 93a98cac7829..33f930e0a0ce 100644 --- a/include/linux/remoteproc/pruss.h +++ b/include/linux/remoteproc/pruss.h @@ -44,6 +44,28 @@ enum pru_ctable_idx { PRU_C31, }; +/* + * enum pruss_mem - PRUSS memory range identifiers + */ +enum pruss_mem { + PRUSS_MEM_DRAM0 = 0, + PRUSS_MEM_DRAM1, + PRUSS_MEM_SHRD_RAM2, + PRUSS_MEM_MAX, +}; + +/** + * struct pruss_mem_region - PRUSS memory region structure + * @va: kernel virtual address of the PRUSS memory region + * @pa: physical (bus) address of the PRUSS memory region + * @size: size of the PRUSS memory region + */ +struct pruss_mem_region { + void __iomem *va; + phys_addr_t pa; + size_t size; +}; + struct device_node; struct rproc; struct pruss; @@ -52,6 +74,10 @@ struct pruss; struct pruss *pruss_get(struct rproc *rproc); void pruss_put(struct pruss *pruss); +int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, + struct pruss_mem_region *region); +int pruss_release_mem_region(struct pruss *pruss, + struct pruss_mem_region *region); #else @@ -62,6 +88,19 @@ static inline struct pruss *pruss_get(struct rproc *rproc) static inline void pruss_put(struct pruss *pruss) { } +static inline int pruss_request_mem_region(struct pruss *pruss, + enum pruss_mem mem_id, + struct pruss_mem_region *region) +{ + return -EOPNOTSUPP; +} + +static inline int pruss_release_mem_region(struct pruss *pruss, + struct pruss_mem_region *region) +{ + return -EOPNOTSUPP; +} + #endif /* CONFIG_TI_PRUSS */ #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)