LinuxLists.cc - [PATCH v4 0/5] Introduce PRU platform consumer API

[permalink] [raw]

Subject: [PATCH v4 1/5] soc: ti: pruss: Add pruss_get()/put() API

From: Tero Kristo <[email protected]>

Add two new get and put API, pruss_get() and pruss_put() to the
PRUSS platform driver to allow client drivers to request a handle
to a PRUSS device. This handle will be used by client drivers to
request various operations of the PRUSS platform driver through
additional API that will be added in the following patches.

The pruss_get() function returns the pruss handle corresponding
to a PRUSS device referenced by a PRU remoteproc instance. The
pruss_put() is the complimentary function to pruss_get().

Co-developed-by: Suman Anna <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
Signed-off-by: Tero Kristo <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
Signed-off-by: MD Danish Anwar <[email protected]>
Reviewed-by: Roger Quadros <[email protected]>
---
drivers/soc/ti/pruss.c | 58 ++++++++++++++++++++++++++++++++
include/linux/pruss_driver.h | 1 +
include/linux/remoteproc/pruss.h | 19 +++++++++++
3 files changed, 78 insertions(+)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 6882c86b3ce5..a169aa1ed044 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -6,6 +6,7 @@
* Author(s):
* Suman Anna <[email protected]>
* Andrew F. Davis <[email protected]>
+ * Tero Kristo <[email protected]>
*/

#include <linux/clk-provider.h>
@@ -18,6 +19,7 @@
#include <linux/pm_runtime.h>
#include <linux/pruss_driver.h>
#include <linux/regmap.h>
+#include <linux/remoteproc.h>
#include <linux/slab.h>

/**
@@ -30,6 +32,62 @@ struct pruss_private_data {
bool has_core_mux_clock;
};

+/**
+ * pruss_get() - get the pruss for a given PRU remoteproc
+ * @rproc: remoteproc handle of a PRU instance
+ *
+ * Finds the parent pruss device for a PRU given the @rproc handle of the
+ * PRU remote processor. This function increments the pruss device's refcount,
+ * so always use pruss_put() to decrement it back once pruss isn't needed
+ * anymore.
+ *
+ * Return: pruss handle on success, and an ERR_PTR on failure using one
+ * of the following error values
+ * -EINVAL if invalid parameter
+ * -ENODEV if PRU device or PRUSS device is not found
+ */
+struct pruss *pruss_get(struct rproc *rproc)
+{
+ struct pruss *pruss;
+ struct device *dev;
+ struct platform_device *ppdev;
+
+ if (IS_ERR_OR_NULL(rproc))
+ return ERR_PTR(-EINVAL);
+
+ dev = &rproc->dev;
+
+ /* make sure it is PRU rproc */
+ if (!dev->parent || !is_pru_rproc(dev->parent))
+ return ERR_PTR(-ENODEV);
+
+ ppdev = to_platform_device(dev->parent->parent);
+ pruss = platform_get_drvdata(ppdev);
+ if (!pruss)
+ return ERR_PTR(-ENODEV);
+
+ get_device(pruss->dev);
+
+ return pruss;
+}
+EXPORT_SYMBOL_GPL(pruss_get);
+
+/**
+ * pruss_put() - decrement pruss device's usecount
+ * @pruss: pruss handle
+ *
+ * Complimentary function for pruss_get(). Needs to be called
+ * after the PRUSS is used, and only if the pruss_get() succeeds.
+ */
+void pruss_put(struct pruss *pruss)
+{
+ if (IS_ERR_OR_NULL(pruss))
+ return;
+
+ put_device(pruss->dev);
+}
+EXPORT_SYMBOL_GPL(pruss_put);
+
static void pruss_of_free_clk_provider(void *data)
{
struct device_node *clk_mux_np = data;
diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
index ecfded30ed05..86242fb5a64a 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_driver.h
@@ -9,6 +9,7 @@
#ifndef _PRUSS_DRIVER_H_
#define _PRUSS_DRIVER_H_

+#include <linux/remoteproc/pruss.h>
#include <linux/types.h>

/*
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 039b50d58df2..93a98cac7829 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -4,12 +4,14 @@
*
* Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
* Suman Anna <[email protected]>
+ * Tero Kristo <[email protected]>
*/

#ifndef __LINUX_PRUSS_H
#define __LINUX_PRUSS_H

#include <linux/device.h>
+#include <linux/err.h>
#include <linux/types.h>

#define PRU_RPROC_DRVNAME "pru-rproc"
@@ -44,6 +46,23 @@ enum pru_ctable_idx {

struct device_node;
struct rproc;
+struct pruss;
+
+#if IS_ENABLED(CONFIG_TI_PRUSS)
+
+struct pruss *pruss_get(struct rproc *rproc);
+void pruss_put(struct pruss *pruss);
+
+#else
+
+static inline struct pruss *pruss_get(struct rproc *rproc)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void pruss_put(struct pruss *pruss) { }
+
+#endif /* CONFIG_TI_PRUSS */

#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)

--
2.25.1

2023-03-13 11:12:55

[permalink] [raw]

Subject: [PATCH v4 3/5] soc: ti: pruss: Add pruss_cfg_read()/update() API

From: Suman Anna <[email protected]>

Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
the PRUSS platform driver to read and program respectively a register
within the PRUSS CFG sub-module represented by a syscon driver.

These APIs are internal to PRUSS driver. Various useful registers
and macros for certain register bit-fields and their values have also
been added.

Signed-off-by: Suman Anna <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
Signed-off-by: MD Danish Anwar <[email protected]>
---
drivers/soc/ti/pruss.c | 39 ++++++++++++++
include/linux/remoteproc/pruss.h | 87 ++++++++++++++++++++++++++++++++
2 files changed, 126 insertions(+)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index c8053c0d735f..26d8129b515c 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -164,6 +164,45 @@ int pruss_release_mem_region(struct pruss *pruss,
}
EXPORT_SYMBOL_GPL(pruss_release_mem_region);

+/**
+ * pruss_cfg_read() - read a PRUSS CFG sub-module register
+ * @pruss: the pruss instance handle
+ * @reg: register offset within the CFG sub-module
+ * @val: pointer to return the value in
+ *
+ * Reads a given register within the PRUSS CFG sub-module and
+ * returns it through the passed-in @val pointer
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+static int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val)
+{
+ if (IS_ERR_OR_NULL(pruss))
+ return -EINVAL;
+
+ return regmap_read(pruss->cfg_regmap, reg, val);
+}
+
+/**
+ * pruss_cfg_update() - configure a PRUSS CFG sub-module register
+ * @pruss: the pruss instance handle
+ * @reg: register offset within the CFG sub-module
+ * @mask: bit mask to use for programming the @val
+ * @val: value to write
+ *
+ * Programs a given register within the PRUSS CFG sub-module
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ if (IS_ERR_OR_NULL(pruss))
+ return -EINVAL;
+
+ return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
+}
+
static void pruss_of_free_clk_provider(void *data)
{
struct device_node *clk_mux_np = data;
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 33f930e0a0ce..12ef10b9fe9a 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -10,12 +10,99 @@
#ifndef __LINUX_PRUSS_H
#define __LINUX_PRUSS_H

+#include <linux/bits.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/types.h>

#define PRU_RPROC_DRVNAME "pru-rproc"

+/*
+ * PRU_ICSS_CFG registers
+ * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
+ */
+#define PRUSS_CFG_REVID 0x00
+#define PRUSS_CFG_SYSCFG 0x04
+#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
+#define PRUSS_CFG_CGR 0x10
+#define PRUSS_CFG_ISRP 0x14
+#define PRUSS_CFG_ISP 0x18
+#define PRUSS_CFG_IESP 0x1C
+#define PRUSS_CFG_IECP 0x20
+#define PRUSS_CFG_SCRP 0x24
+#define PRUSS_CFG_PMAO 0x28
+#define PRUSS_CFG_MII_RT 0x2C
+#define PRUSS_CFG_IEPCLK 0x30
+#define PRUSS_CFG_SPP 0x34
+#define PRUSS_CFG_PIN_MX 0x40
+
+/* PRUSS_GPCFG register bits */
+#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
+
+#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
+#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
+
+#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
+#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
+
+#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
+#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
+#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
+
+#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
+
+#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
+#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
+
+#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
+#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
+
+#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
+#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
+#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
+
+#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
+#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
+
+#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
+#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
+
+/* PRUSS_MII_RT register bits */
+#define PRUSS_MII_RT_EVENT_EN BIT(0)
+
+/* PRUSS_SPP register bits */
+#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
+#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)
+
+/*
+ * enum pruss_gp_mux_sel - PRUSS GPI/O Mux modes for the
+ * PRUSS_GPCFG0/1 registers
+ *
+ * NOTE: The below defines are the most common values, but there
+ * are some exceptions like on 66AK2G, where the RESERVED and MII2
+ * values are interchanged. Also, this bit-field does not exist on
+ * AM335x SoCs
+ */
+enum pruss_gp_mux_sel {
+ PRUSS_GP_MUX_SEL_GP = 0,
+ PRUSS_GP_MUX_SEL_ENDAT,
+ PRUSS_GP_MUX_SEL_RESERVED,
+ PRUSS_GP_MUX_SEL_SD,
+ PRUSS_GP_MUX_SEL_MII2,
+ PRUSS_GP_MUX_SEL_MAX,
+};
+
+/*
+ * enum pruss_gpi_mode - PRUSS GPI configuration modes, used
+ * to program the PRUSS_GPCFG0/1 registers
+ */
+enum pruss_gpi_mode {
+ PRUSS_GPI_MODE_DIRECT = 0,
+ PRUSS_GPI_MODE_PARALLEL,
+ PRUSS_GPI_MODE_28BIT_SHIFT,
+ PRUSS_GPI_MODE_MII,
+};
+
/**
* enum pruss_pru_id - PRU core identifiers
* @PRUSS_PRU0: PRU Core 0.
--
2.25.1

2023-03-13 11:13:06

[permalink] [raw]

Subject: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

From: "Andrew F. Davis" <[email protected]>

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 <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
Signed-off-by: Andrew F. Davis <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: MD Danish Anwar <[email protected]>
Reviewed-by: Roger Quadros <[email protected]>
---
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(-)

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)
--
2.25.1

2023-03-13 11:13:11

[permalink] [raw]

Subject: [PATCH v4 5/5] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

From: Tero Kristo <[email protected]>

Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
to get and set the GP MUX mode for programming the PRUSS internal wrapper
mux functionality as needed by usecases.

Co-developed-by: Suman Anna <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
Signed-off-by: Tero Kristo <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
Signed-off-by: MD Danish Anwar <[email protected]>
Reviewed-by: Roger Quadros <[email protected]>
---
drivers/soc/ti/pruss.c | 44 ++++++++++++++++++++++++++++++++
include/linux/remoteproc/pruss.h | 12 +++++++++
2 files changed, 56 insertions(+)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 2f04b7922ddb..90f81f4d77fb 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -263,6 +263,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)
}
EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable);

+/**
+ * pruss_cfg_get_gpmux() - get the current GPMUX value for a PRU device
+ * @pruss: pruss instance
+ * @pru_id: PRU identifier (0-1)
+ * @mux: pointer to store the current mux value into
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+int pruss_cfg_get_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 *mux)
+{
+ int ret = 0;
+ u32 val;
+
+ if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
+ return -EINVAL;
+
+ ret = pruss_cfg_read(pruss, PRUSS_CFG_GPCFG(pru_id), &val);
+ if (!ret)
+ *mux = (u8)((val & PRUSS_GPCFG_PRU_MUX_SEL_MASK) >>
+ PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_get_gpmux);
+
+/**
+ * pruss_cfg_set_gpmux() - set the GPMUX value for a PRU device
+ * @pruss: pruss instance
+ * @pru_id: PRU identifier (0-1)
+ * @mux: new mux value for PRU
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+int pruss_cfg_set_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 mux)
+{
+ if (mux >= PRUSS_GP_MUX_SEL_MAX ||
+ pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
+ return -EINVAL;
+
+ return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
+ PRUSS_GPCFG_PRU_MUX_SEL_MASK,
+ (u32)mux << PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_set_gpmux);
+
static void pruss_of_free_clk_provider(void *data)
{
struct device_node *clk_mux_np = data;
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 51a3eedd2be6..4138a9fcc3ad 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -170,6 +170,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
enum pruss_gpi_mode mode);
int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask);
+int pruss_cfg_get_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 *mux);
+int pruss_cfg_set_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 mux);

#else

@@ -210,6 +212,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mas
return ERR_PTR(-EOPNOTSUPP);
}

+static inline int pruss_cfg_get_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 *mux)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline int pruss_cfg_set_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 mux)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
#endif /* CONFIG_TI_PRUSS */

#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
--
2.25.1

2023-03-13 11:13:16

[permalink] [raw]

Subject: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

From: Suman Anna <[email protected]>

The PRUSS CFG module is represented as a syscon node and is currently
managed by the PRUSS platform driver. Add easy accessor functions to set
GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
to enable the PRUSS Ethernet usecase. These functions reuse the generic
pruss_cfg_update() API function.

Signed-off-by: Suman Anna <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
Signed-off-by: MD Danish Anwar <[email protected]>
---
drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
include/linux/remoteproc/pruss.h | 22 ++++++++++++
2 files changed, 82 insertions(+)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 26d8129b515c..2f04b7922ddb 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
}

+/**
+ * pruss_cfg_gpimode() - set the GPI mode of the PRU
+ * @pruss: the pruss instance handle
+ * @pru_id: id of the PRU core within the PRUSS
+ * @mode: GPI mode to set
+ *
+ * Sets the GPI mode for a given PRU by programming the
+ * corresponding PRUSS_CFG_GPCFGx register
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
+ enum pruss_gpi_mode mode)
+{
+ if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
+ return -EINVAL;
+
+ if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
+ return -EINVAL;
+
+ return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
+ PRUSS_GPCFG_PRU_GPI_MODE_MASK,
+ mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
+
+/**
+ * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
+ * @pruss: the pruss instance
+ * @enable: enable/disable
+ *
+ * Enable/disable the MII RT Events for the PRUSS.
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
+{
+ u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
+
+ return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
+ PRUSS_MII_RT_EVENT_EN, set);
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
+
+/**
+ * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
+ * @pruss: the pruss instance
+ * @enable: enable/disable
+ * @mask: Mask for PRU / RTU
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)
+{
+ u32 set = enable ? mask : 0;
+
+ return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable);
+
static void pruss_of_free_clk_provider(void *data)
{
struct device_node *clk_mux_np = data;
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 12ef10b9fe9a..51a3eedd2be6 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -101,6 +101,7 @@ enum pruss_gpi_mode {
PRUSS_GPI_MODE_PARALLEL,
PRUSS_GPI_MODE_28BIT_SHIFT,
PRUSS_GPI_MODE_MII,
+ PRUSS_GPI_MODE_MAX,
};

/**
@@ -165,6 +166,10 @@ 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);
+int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
+ enum pruss_gpi_mode mode);
+int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
+int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask);

#else

@@ -188,6 +193,23 @@ static inline int pruss_release_mem_region(struct pruss *pruss,
return -EOPNOTSUPP;
}

+static inline int pruss_cfg_gpimode(struct pruss *pruss,
+ enum pruss_pru_id pru_id,
+ enum pruss_gpi_mode mode)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
#endif /* CONFIG_TI_PRUSS */

#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
--
2.25.1

2023-03-13 16:19:32

by Andrew Davis

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] soc: ti: pruss: Add pruss_get()/put() API

On 3/13/23 6:11 AM, MD Danish Anwar wrote:
> From: Tero Kristo <[email protected]>
>

To prevent bounced emails from folks seeing the author name here
and in code below, suggest using Tero's kernel.org email everywhere:

Tero Kristo <[email protected]>

Andrew

2023-03-15 12:08:28

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] soc: ti: pruss: Add pruss_cfg_read()/update() API

Danish,

On 13/03/2023 13:11, MD Danish Anwar wrote:
> From: Suman Anna <[email protected]>
>
> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
> the PRUSS platform driver to read and program respectively a register
> within the PRUSS CFG sub-module represented by a syscon driver.
>
> These APIs are internal to PRUSS driver. Various useful registers
> and macros for certain register bit-fields and their values have also
> been added.
>
> Signed-off-by: Suman Anna <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>
> Signed-off-by: MD Danish Anwar <[email protected]>
> ---
> drivers/soc/ti/pruss.c | 39 ++++++++++++++
> include/linux/remoteproc/pruss.h | 87 ++++++++++++++++++++++++++++++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index c8053c0d735f..26d8129b515c 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -164,6 +164,45 @@ int pruss_release_mem_region(struct pruss *pruss,
> }
> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>
> +/**
> + * pruss_cfg_read() - read a PRUSS CFG sub-module register
> + * @pruss: the pruss instance handle
> + * @reg: register offset within the CFG sub-module
> + * @val: pointer to return the value in
> + *
> + * Reads a given register within the PRUSS CFG sub-module and
> + * returns it through the passed-in @val pointer
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +static int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val)
> +{
> + if (IS_ERR_OR_NULL(pruss))
> + return -EINVAL;
> +
> + return regmap_read(pruss->cfg_regmap, reg, val);
> +}
> +
> +/**
> + * pruss_cfg_update() - configure a PRUSS CFG sub-module register
> + * @pruss: the pruss instance handle
> + * @reg: register offset within the CFG sub-module
> + * @mask: bit mask to use for programming the @val
> + * @val: value to write
> + *
> + * Programs a given register within the PRUSS CFG sub-module
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + if (IS_ERR_OR_NULL(pruss))
> + return -EINVAL;
> +
> + return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
> +}
> +
> static void pruss_of_free_clk_provider(void *data)
> {
> struct device_node *clk_mux_np = data;
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> index 33f930e0a0ce..12ef10b9fe9a 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -10,12 +10,99 @@
> #ifndef __LINUX_PRUSS_H
> #define __LINUX_PRUSS_H
>
> +#include <linux/bits.h>
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/types.h>
>
> #define PRU_RPROC_DRVNAME "pru-rproc"
>
> +/*
> + * PRU_ICSS_CFG registers
> + * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
> + */
> +#define PRUSS_CFG_REVID 0x00
> +#define PRUSS_CFG_SYSCFG 0x04
> +#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
> +#define PRUSS_CFG_CGR 0x10
> +#define PRUSS_CFG_ISRP 0x14
> +#define PRUSS_CFG_ISP 0x18
> +#define PRUSS_CFG_IESP 0x1C
> +#define PRUSS_CFG_IECP 0x20
> +#define PRUSS_CFG_SCRP 0x24
> +#define PRUSS_CFG_PMAO 0x28
> +#define PRUSS_CFG_MII_RT 0x2C
> +#define PRUSS_CFG_IEPCLK 0x30
> +#define PRUSS_CFG_SPP 0x34
> +#define PRUSS_CFG_PIN_MX 0x40
> +
> +/* PRUSS_GPCFG register bits */
> +#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
> +
> +#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
> +#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
> +
> +#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
> +#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
> +
> +#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
> +#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
> +#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
> +
> +#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
> +
> +#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
> +#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
> +
> +#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
> +#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
> +
> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
> +
> +#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
> +#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
> +
> +#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
> +#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
> +
> +/* PRUSS_MII_RT register bits */
> +#define PRUSS_MII_RT_EVENT_EN BIT(0)
> +
> +/* PRUSS_SPP register bits */
> +#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
> +#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)

Can we please move all the above definitions to private driver/soc/ti/pruss.h?
You can also add pruss_cfg_read and pruss_cfg_update there.

> +
> +/*
> + * enum pruss_gp_mux_sel - PRUSS GPI/O Mux modes for the
> + * PRUSS_GPCFG0/1 registers
> + *
> + * NOTE: The below defines are the most common values, but there
> + * are some exceptions like on 66AK2G, where the RESERVED and MII2
> + * values are interchanged. Also, this bit-field does not exist on
> + * AM335x SoCs
> + */
> +enum pruss_gp_mux_sel {
> + PRUSS_GP_MUX_SEL_GP = 0,
> + PRUSS_GP_MUX_SEL_ENDAT,
> + PRUSS_GP_MUX_SEL_RESERVED,
> + PRUSS_GP_MUX_SEL_SD,
> + PRUSS_GP_MUX_SEL_MII2,
> + PRUSS_GP_MUX_SEL_MAX,
> +};
> +
> +/*
> + * enum pruss_gpi_mode - PRUSS GPI configuration modes, used
> + * to program the PRUSS_GPCFG0/1 registers
> + */
> +enum pruss_gpi_mode {
> + PRUSS_GPI_MODE_DIRECT = 0,
> + PRUSS_GPI_MODE_PARALLEL,
> + PRUSS_GPI_MODE_28BIT_SHIFT,
> + PRUSS_GPI_MODE_MII,
> +};
> +
> /**
> * enum pruss_pru_id - PRU core identifiers
> * @PRUSS_PRU0: PRU Core 0.

cheers,
-roger

2023-03-15 12:24:45

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 13/03/2023 13:11, MD Danish Anwar wrote:
> From: Suman Anna <[email protected]>
>
> The PRUSS CFG module is represented as a syscon node and is currently
> managed by the PRUSS platform driver. Add easy accessor functions to set
> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
> to enable the PRUSS Ethernet usecase. These functions reuse the generic
> pruss_cfg_update() API function.
>
> Signed-off-by: Suman Anna <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>
> Signed-off-by: MD Danish Anwar <[email protected]>
> ---
> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
> include/linux/remoteproc/pruss.h | 22 ++++++++++++
> 2 files changed, 82 insertions(+)
>
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 26d8129b515c..2f04b7922ddb 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
> }
>
> +/**
> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
> + * @pruss: the pruss instance handle
> + * @pru_id: id of the PRU core within the PRUSS
> + * @mode: GPI mode to set
> + *
> + * Sets the GPI mode for a given PRU by programming the
> + * corresponding PRUSS_CFG_GPCFGx register
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
> + enum pruss_gpi_mode mode)
> +{
> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
> + return -EINVAL;
> +
> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
> + return -EINVAL;
> +
> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
> +}
> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
> +
> +/**
> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
> + * @pruss: the pruss instance
> + * @enable: enable/disable
> + *
> + * Enable/disable the MII RT Events for the PRUSS.
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
> +{
> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
> +
> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
> + PRUSS_MII_RT_EVENT_EN, set);
> +}
> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
> +
> +/**
> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
> + * @pruss: the pruss instance
> + * @enable: enable/disable
> + * @mask: Mask for PRU / RTU

You should not expect the user to provide the mask but only
the core type e.g.

enum pru_type {
PRU_TYPE_PRU = 0,
PRU_TYPE_RTU,
PRU_TYPE_TX_PRU,
PRU_TYPE_MAX,
};

Then you figure out the mask in the function.
Also check for invalid pru_type and return error if so.

> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)

re-arrange so it is (struct pruss, enum pru_type, bool enable)

> +{
> + u32 set = enable ? mask : 0;
> +
> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
> +}
> +EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable);
> +
> static void pruss_of_free_clk_provider(void *data)
> {
> struct device_node *clk_mux_np = data;
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> index 12ef10b9fe9a..51a3eedd2be6 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -101,6 +101,7 @@ enum pruss_gpi_mode {
> PRUSS_GPI_MODE_PARALLEL,
> PRUSS_GPI_MODE_28BIT_SHIFT,
> PRUSS_GPI_MODE_MII,
> + PRUSS_GPI_MODE_MAX,

This could have come as part of patch 3.

> };
>
> /**
> @@ -165,6 +166,10 @@ 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);
> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
> + enum pruss_gpi_mode mode);
> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
> +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask);
>
> #else
>
> @@ -188,6 +193,23 @@ static inline int pruss_release_mem_region(struct pruss *pruss,
> return -EOPNOTSUPP;
> }
>
> +static inline int pruss_cfg_gpimode(struct pruss *pruss,
> + enum pruss_pru_id pru_id,
> + enum pruss_gpi_mode mode)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> #endif /* CONFIG_TI_PRUSS */
>
> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)

cheers,
-roger

2023-03-16 11:05:26

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

Hi Roger,

On 15/03/23 17:52, Roger Quadros wrote:
>
>
> On 13/03/2023 13:11, MD Danish Anwar wrote:
>> From: Suman Anna <[email protected]>
>>
>> The PRUSS CFG module is represented as a syscon node and is currently
>> managed by the PRUSS platform driver. Add easy accessor functions to set
>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>> pruss_cfg_update() API function.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> Signed-off-by: MD Danish Anwar <[email protected]>
>> ---
>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
>> include/linux/remoteproc/pruss.h | 22 ++++++++++++
>> 2 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 26d8129b515c..2f04b7922ddb 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>> }
>>
>> +/**
>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
>> + * @pruss: the pruss instance handle
>> + * @pru_id: id of the PRU core within the PRUSS
>> + * @mode: GPI mode to set
>> + *
>> + * Sets the GPI mode for a given PRU by programming the
>> + * corresponding PRUSS_CFG_GPCFGx register
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>> + enum pruss_gpi_mode mode)
>> +{
>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>> + return -EINVAL;
>> +
>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>> + return -EINVAL;
>> +
>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
>> +}
>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
>> +
>> +/**
>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
>> + * @pruss: the pruss instance
>> + * @enable: enable/disable
>> + *
>> + * Enable/disable the MII RT Events for the PRUSS.
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>> +{
>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
>> +
>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
>> + PRUSS_MII_RT_EVENT_EN, set);
>> +}
>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
>> +
>> +/**
>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>> + * @pruss: the pruss instance
>> + * @enable: enable/disable
>> + * @mask: Mask for PRU / RTU
>
> You should not expect the user to provide the mask but only
> the core type e.g.
>
> enum pru_type {
> PRU_TYPE_PRU = 0,
> PRU_TYPE_RTU,
> PRU_TYPE_TX_PRU,
> PRU_TYPE_MAX,
> };
>
> Then you figure out the mask in the function.
> Also check for invalid pru_type and return error if so.
>

Sure Roger, I will create a enum and take it as parameter in API. Based on
these enum I will calculate mask and do XFR shifting inside the API
pruss_cfg_xfr_enable().

There are two registers for XFR shift.

#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
#define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)

For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN,
for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU
shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN)

So the enum would be something like this.

/**
* enum xfr_shift_type - XFR shift type
* @XFR_SHIFT_PRU: Enables XFR shift for PRU
* @XFR_SHIFT_RTU: Enables XFR shift for RTU
* @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU
* @XFR_SHIFT_MAX: Total number of XFR shift types available.
*
*/

enum xfr_shift_type {
XFR_SHIFT_PRU = 0,
XFR_SHIFT_RTU,
XFR_SHIFT_PRU_RTU,
XFR_SHIFT_MAX,
};

In pruss_cfg_xfr_enable() API, I will use switch case, and for first three
enums, I will calculate the mask.

If input is anything other than first three, I will retun -EINVAL. This will
serve as check for valid xfr_shift_type.

The API will look like this.

int pruss_cfg_xfr_enable(struct pruss *pruss, enum xfr_shift_type xfr_type,
bool enable);
{
u32 mask;

switch (xfr_type) {
case XFR_SHIFT_PRU:
mask = PRUSS_SPP_XFER_SHIFT_EN;
break;
case XFR_SHIFT_RTU:
mask = PRUSS_SPP_RTU_XFR_SHIFT_EN;
break;
case XFR_SHIFT_PRU_RTU:
mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
break;
default:
return -EINVAL;
}

u32 set = enable ? mask : 0;

return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
}

This entire change I will keep as part of this patch only.

Please let me know if this looks OK to you.

>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)
>
> re-arrange so it is (struct pruss, enum pru_type, bool enable)
>
>> +{
>> + u32 set = enable ? mask : 0;
>> +
>> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
>> +}
>> +EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable);
>> +
>> static void pruss_of_free_clk_provider(void *data)
>> {
>> struct device_node *clk_mux_np = data;
>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>> index 12ef10b9fe9a..51a3eedd2be6 100644
>> --- a/include/linux/remoteproc/pruss.h
>> +++ b/include/linux/remoteproc/pruss.h
>> @@ -101,6 +101,7 @@ enum pruss_gpi_mode {
>> PRUSS_GPI_MODE_PARALLEL,
>> PRUSS_GPI_MODE_28BIT_SHIFT,
>> PRUSS_GPI_MODE_MII,
>> + PRUSS_GPI_MODE_MAX,
>
> This could have come as part of patch 3.
>
>> };
>>
>> /**
>> @@ -165,6 +166,10 @@ 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);
>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>> + enum pruss_gpi_mode mode);
>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
>> +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask);
>>
>> #else
>>
>> @@ -188,6 +193,23 @@ static inline int pruss_release_mem_region(struct pruss *pruss,
>> return -EOPNOTSUPP;
>> }
>>
>> +static inline int pruss_cfg_gpimode(struct pruss *pruss,
>> + enum pruss_pru_id pru_id,
>> + enum pruss_gpi_mode mode)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> +static inline int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> #endif /* CONFIG_TI_PRUSS */
>>
>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-16 11:08:32

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 3/5] soc: ti: pruss: Add pruss_cfg_read()/update() API

On 15/03/23 17:37, Roger Quadros wrote:
> Danish,
>
> On 13/03/2023 13:11, MD Danish Anwar wrote:
>> From: Suman Anna <[email protected]>
>>
>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>> the PRUSS platform driver to read and program respectively a register
>> within the PRUSS CFG sub-module represented by a syscon driver.
>>
>> These APIs are internal to PRUSS driver. Various useful registers
>> and macros for certain register bit-fields and their values have also
>> been added.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> Signed-off-by: MD Danish Anwar <[email protected]>
>> ---
>> drivers/soc/ti/pruss.c | 39 ++++++++++++++
>> include/linux/remoteproc/pruss.h | 87 ++++++++++++++++++++++++++++++++
>> 2 files changed, 126 insertions(+)
>>
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index c8053c0d735f..26d8129b515c 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -164,6 +164,45 @@ int pruss_release_mem_region(struct pruss *pruss,
>> }
>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>
>> +/**
>> + * pruss_cfg_read() - read a PRUSS CFG sub-module register
>> + * @pruss: the pruss instance handle
>> + * @reg: register offset within the CFG sub-module
>> + * @val: pointer to return the value in
>> + *
>> + * Reads a given register within the PRUSS CFG sub-module and
>> + * returns it through the passed-in @val pointer
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +static int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val)
>> +{
>> + if (IS_ERR_OR_NULL(pruss))
>> + return -EINVAL;
>> +
>> + return regmap_read(pruss->cfg_regmap, reg, val);
>> +}
>> +
>> +/**
>> + * pruss_cfg_update() - configure a PRUSS CFG sub-module register
>> + * @pruss: the pruss instance handle
>> + * @reg: register offset within the CFG sub-module
>> + * @mask: bit mask to use for programming the @val
>> + * @val: value to write
>> + *
>> + * Programs a given register within the PRUSS CFG sub-module
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>> + unsigned int mask, unsigned int val)
>> +{
>> + if (IS_ERR_OR_NULL(pruss))
>> + return -EINVAL;
>> +
>> + return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>> +}
>> +
>> static void pruss_of_free_clk_provider(void *data)
>> {
>> struct device_node *clk_mux_np = data;
>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>> index 33f930e0a0ce..12ef10b9fe9a 100644
>> --- a/include/linux/remoteproc/pruss.h
>> +++ b/include/linux/remoteproc/pruss.h
>> @@ -10,12 +10,99 @@
>> #ifndef __LINUX_PRUSS_H
>> #define __LINUX_PRUSS_H
>>
>> +#include <linux/bits.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> #include <linux/types.h>
>>
>> #define PRU_RPROC_DRVNAME "pru-rproc"
>>
>> +/*
>> + * PRU_ICSS_CFG registers
>> + * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
>> + */
>> +#define PRUSS_CFG_REVID 0x00
>> +#define PRUSS_CFG_SYSCFG 0x04
>> +#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
>> +#define PRUSS_CFG_CGR 0x10
>> +#define PRUSS_CFG_ISRP 0x14
>> +#define PRUSS_CFG_ISP 0x18
>> +#define PRUSS_CFG_IESP 0x1C
>> +#define PRUSS_CFG_IECP 0x20
>> +#define PRUSS_CFG_SCRP 0x24
>> +#define PRUSS_CFG_PMAO 0x28
>> +#define PRUSS_CFG_MII_RT 0x2C
>> +#define PRUSS_CFG_IEPCLK 0x30
>> +#define PRUSS_CFG_SPP 0x34
>> +#define PRUSS_CFG_PIN_MX 0x40
>> +
>> +/* PRUSS_GPCFG register bits */
>> +#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
>> +
>> +#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
>> +#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
>> +
>> +#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
>> +#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
>> +
>> +#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
>> +#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
>> +#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
>> +#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
>> +
>> +#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
>> +#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
>> +
>> +/* PRUSS_MII_RT register bits */
>> +#define PRUSS_MII_RT_EVENT_EN BIT(0)
>> +
>> +/* PRUSS_SPP register bits */
>> +#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>> +#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)
>
> Can we please move all the above definitions to private driver/soc/ti/pruss.h?
> You can also add pruss_cfg_read and pruss_cfg_update there.
>

Sure Roger, I'll move all these definitions to pruss.h

>> +
>> +/*
>> + * enum pruss_gp_mux_sel - PRUSS GPI/O Mux modes for the
>> + * PRUSS_GPCFG0/1 registers
>> + *
>> + * NOTE: The below defines are the most common values, but there
>> + * are some exceptions like on 66AK2G, where the RESERVED and MII2
>> + * values are interchanged. Also, this bit-field does not exist on
>> + * AM335x SoCs
>> + */
>> +enum pruss_gp_mux_sel {
>> + PRUSS_GP_MUX_SEL_GP = 0,
>> + PRUSS_GP_MUX_SEL_ENDAT,
>> + PRUSS_GP_MUX_SEL_RESERVED,
>> + PRUSS_GP_MUX_SEL_SD,
>> + PRUSS_GP_MUX_SEL_MII2,
>> + PRUSS_GP_MUX_SEL_MAX,
>> +};
>> +
>> +/*
>> + * enum pruss_gpi_mode - PRUSS GPI configuration modes, used
>> + * to program the PRUSS_GPCFG0/1 registers
>> + */
>> +enum pruss_gpi_mode {
>> + PRUSS_GPI_MODE_DIRECT = 0,
>> + PRUSS_GPI_MODE_PARALLEL,
>> + PRUSS_GPI_MODE_28BIT_SHIFT,
>> + PRUSS_GPI_MODE_MII,
>> +};
>> +
>> /**
>> * enum pruss_pru_id - PRU core identifiers
>> * @PRUSS_PRU0: PRU Core 0.
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-16 11:31:02

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 3/5] soc: ti: pruss: Add pruss_cfg_read()/update() API

Roger,

On 16/03/23 16:38, Md Danish Anwar wrote:
>
> On 15/03/23 17:37, Roger Quadros wrote:
>> Danish,
>>
>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>> From: Suman Anna <[email protected]>
>>>
>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>> the PRUSS platform driver to read and program respectively a register
>>> within the PRUSS CFG sub-module represented by a syscon driver.
>>>
>>> These APIs are internal to PRUSS driver. Various useful registers
>>> and macros for certain register bit-fields and their values have also
>>> been added.
>>>
>>> Signed-off-by: Suman Anna <[email protected]>
>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>> ---
>>> drivers/soc/ti/pruss.c | 39 ++++++++++++++
>>> include/linux/remoteproc/pruss.h | 87 ++++++++++++++++++++++++++++++++
>>> 2 files changed, 126 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>> index c8053c0d735f..26d8129b515c 100644
>>> --- a/drivers/soc/ti/pruss.c
>>> +++ b/drivers/soc/ti/pruss.c
>>> @@ -164,6 +164,45 @@ int pruss_release_mem_region(struct pruss *pruss,
>>> }
>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>
>>> +/**
>>> + * pruss_cfg_read() - read a PRUSS CFG sub-module register
>>> + * @pruss: the pruss instance handle
>>> + * @reg: register offset within the CFG sub-module
>>> + * @val: pointer to return the value in
>>> + *
>>> + * Reads a given register within the PRUSS CFG sub-module and
>>> + * returns it through the passed-in @val pointer
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +static int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val)
>>> +{
>>> + if (IS_ERR_OR_NULL(pruss))
>>> + return -EINVAL;
>>> +
>>> + return regmap_read(pruss->cfg_regmap, reg, val);
>>> +}
>>> +
>>> +/**
>>> + * pruss_cfg_update() - configure a PRUSS CFG sub-module register
>>> + * @pruss: the pruss instance handle
>>> + * @reg: register offset within the CFG sub-module
>>> + * @mask: bit mask to use for programming the @val
>>> + * @val: value to write
>>> + *
>>> + * Programs a given register within the PRUSS CFG sub-module
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>> + unsigned int mask, unsigned int val)
>>> +{
>>> + if (IS_ERR_OR_NULL(pruss))
>>> + return -EINVAL;
>>> +
>>> + return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>> +}
>>> +
>>> static void pruss_of_free_clk_provider(void *data)
>>> {
>>> struct device_node *clk_mux_np = data;
>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>> index 33f930e0a0ce..12ef10b9fe9a 100644
>>> --- a/include/linux/remoteproc/pruss.h
>>> +++ b/include/linux/remoteproc/pruss.h
>>> @@ -10,12 +10,99 @@
>>> #ifndef __LINUX_PRUSS_H
>>> #define __LINUX_PRUSS_H
>>>
>>> +#include <linux/bits.h>
>>> #include <linux/device.h>
>>> #include <linux/err.h>
>>> #include <linux/types.h>
>>>
>>> #define PRU_RPROC_DRVNAME "pru-rproc"
>>>
>>> +/*
>>> + * PRU_ICSS_CFG registers
>>> + * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
>>> + */
>>> +#define PRUSS_CFG_REVID 0x00
>>> +#define PRUSS_CFG_SYSCFG 0x04
>>> +#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
>>> +#define PRUSS_CFG_CGR 0x10
>>> +#define PRUSS_CFG_ISRP 0x14
>>> +#define PRUSS_CFG_ISP 0x18
>>> +#define PRUSS_CFG_IESP 0x1C
>>> +#define PRUSS_CFG_IECP 0x20
>>> +#define PRUSS_CFG_SCRP 0x24
>>> +#define PRUSS_CFG_PMAO 0x28
>>> +#define PRUSS_CFG_MII_RT 0x2C
>>> +#define PRUSS_CFG_IEPCLK 0x30
>>> +#define PRUSS_CFG_SPP 0x34
>>> +#define PRUSS_CFG_PIN_MX 0x40
>>> +
>>> +/* PRUSS_GPCFG register bits */
>>> +#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
>>> +
>>> +#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
>>> +#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
>>> +
>>> +#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
>>> +#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
>>> +
>>> +#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
>>> +#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
>>> +#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
>>> +
>>> +#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
>>> +
>>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
>>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
>>> +
>>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
>>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
>>> +
>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
>>> +
>>> +#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
>>> +#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
>>> +
>>> +#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
>>> +#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
>>> +
>>> +/* PRUSS_MII_RT register bits */
>>> +#define PRUSS_MII_RT_EVENT_EN BIT(0)
>>> +
>>> +/* PRUSS_SPP register bits */
>>> +#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>>> +#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)
>>
>> Can we please move all the above definitions to private driver/soc/ti/pruss.h?
>> You can also add pruss_cfg_read and pruss_cfg_update there.
>>

There is no driver/soc/ti/pruss.h. The pruss.h file is located in
include/linux/remoteproc/pruss.h and there is one pruss_driver.h file which is
located in include/linux/pruss_driver.h

Do you want me to create another header file at driver/soc/ti/pruss.h and place
all these definitions inside that?

Please let me know.

>
> Sure Roger, I'll move all these definitions to pruss.h
>
>>> +
>>> +/*
>>> + * enum pruss_gp_mux_sel - PRUSS GPI/O Mux modes for the
>>> + * PRUSS_GPCFG0/1 registers
>>> + *
>>> + * NOTE: The below defines are the most common values, but there
>>> + * are some exceptions like on 66AK2G, where the RESERVED and MII2
>>> + * values are interchanged. Also, this bit-field does not exist on
>>> + * AM335x SoCs
>>> + */
>>> +enum pruss_gp_mux_sel {
>>> + PRUSS_GP_MUX_SEL_GP = 0,
>>> + PRUSS_GP_MUX_SEL_ENDAT,
>>> + PRUSS_GP_MUX_SEL_RESERVED,
>>> + PRUSS_GP_MUX_SEL_SD,
>>> + PRUSS_GP_MUX_SEL_MII2,
>>> + PRUSS_GP_MUX_SEL_MAX,
>>> +};
>>> +
>>> +/*
>>> + * enum pruss_gpi_mode - PRUSS GPI configuration modes, used
>>> + * to program the PRUSS_GPCFG0/1 registers
>>> + */
>>> +enum pruss_gpi_mode {
>>> + PRUSS_GPI_MODE_DIRECT = 0,
>>> + PRUSS_GPI_MODE_PARALLEL,
>>> + PRUSS_GPI_MODE_28BIT_SHIFT,
>>> + PRUSS_GPI_MODE_MII,
>>> +};
>>> +
>>> /**
>>> * enum pruss_pru_id - PRU core identifiers
>>> * @PRUSS_PRU0: PRU Core 0.
>>
>> cheers,
>> -roger
>

--
Thanks and Regards,
Danish.

2023-03-16 11:33:13

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 3/5] soc: ti: pruss: Add pruss_cfg_read()/update() API

On 16/03/2023 13:29, Md Danish Anwar wrote:
> Roger,
>
> On 16/03/23 16:38, Md Danish Anwar wrote:
>>
>> On 15/03/23 17:37, Roger Quadros wrote:
>>> Danish,
>>>
>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>> From: Suman Anna <[email protected]>
>>>>
>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>> the PRUSS platform driver to read and program respectively a register
>>>> within the PRUSS CFG sub-module represented by a syscon driver.
>>>>
>>>> These APIs are internal to PRUSS driver. Various useful registers
>>>> and macros for certain register bit-fields and their values have also
>>>> been added.
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>> ---
>>>> drivers/soc/ti/pruss.c | 39 ++++++++++++++
>>>> include/linux/remoteproc/pruss.h | 87 ++++++++++++++++++++++++++++++++
>>>> 2 files changed, 126 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index c8053c0d735f..26d8129b515c 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -164,6 +164,45 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>> }
>>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>>
>>>> +/**
>>>> + * pruss_cfg_read() - read a PRUSS CFG sub-module register
>>>> + * @pruss: the pruss instance handle
>>>> + * @reg: register offset within the CFG sub-module
>>>> + * @val: pointer to return the value in
>>>> + *
>>>> + * Reads a given register within the PRUSS CFG sub-module and
>>>> + * returns it through the passed-in @val pointer
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise
>>>> + */
>>>> +static int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val)
>>>> +{
>>>> + if (IS_ERR_OR_NULL(pruss))
>>>> + return -EINVAL;
>>>> +
>>>> + return regmap_read(pruss->cfg_regmap, reg, val);
>>>> +}
>>>> +
>>>> +/**
>>>> + * pruss_cfg_update() - configure a PRUSS CFG sub-module register
>>>> + * @pruss: the pruss instance handle
>>>> + * @reg: register offset within the CFG sub-module
>>>> + * @mask: bit mask to use for programming the @val
>>>> + * @val: value to write
>>>> + *
>>>> + * Programs a given register within the PRUSS CFG sub-module
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise
>>>> + */
>>>> +static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> + unsigned int mask, unsigned int val)
>>>> +{
>>>> + if (IS_ERR_OR_NULL(pruss))
>>>> + return -EINVAL;
>>>> +
>>>> + return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>>> +}
>>>> +
>>>> static void pruss_of_free_clk_provider(void *data)
>>>> {
>>>> struct device_node *clk_mux_np = data;
>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>> index 33f930e0a0ce..12ef10b9fe9a 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -10,12 +10,99 @@
>>>> #ifndef __LINUX_PRUSS_H
>>>> #define __LINUX_PRUSS_H
>>>>
>>>> +#include <linux/bits.h>
>>>> #include <linux/device.h>
>>>> #include <linux/err.h>
>>>> #include <linux/types.h>
>>>>
>>>> #define PRU_RPROC_DRVNAME "pru-rproc"
>>>>
>>>> +/*
>>>> + * PRU_ICSS_CFG registers
>>>> + * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
>>>> + */
>>>> +#define PRUSS_CFG_REVID 0x00
>>>> +#define PRUSS_CFG_SYSCFG 0x04
>>>> +#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
>>>> +#define PRUSS_CFG_CGR 0x10
>>>> +#define PRUSS_CFG_ISRP 0x14
>>>> +#define PRUSS_CFG_ISP 0x18
>>>> +#define PRUSS_CFG_IESP 0x1C
>>>> +#define PRUSS_CFG_IECP 0x20
>>>> +#define PRUSS_CFG_SCRP 0x24
>>>> +#define PRUSS_CFG_PMAO 0x28
>>>> +#define PRUSS_CFG_MII_RT 0x2C
>>>> +#define PRUSS_CFG_IEPCLK 0x30
>>>> +#define PRUSS_CFG_SPP 0x34
>>>> +#define PRUSS_CFG_PIN_MX 0x40
>>>> +
>>>> +/* PRUSS_GPCFG register bits */
>>>> +#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
>>>> +#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
>>>> +#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
>>>> +#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
>>>> +#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
>>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
>>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
>>>> +#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
>>>> +
>>>> +#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
>>>> +#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
>>>> +
>>>> +/* PRUSS_MII_RT register bits */
>>>> +#define PRUSS_MII_RT_EVENT_EN BIT(0)
>>>> +
>>>> +/* PRUSS_SPP register bits */
>>>> +#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>>>> +#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)
>>>
>>> Can we please move all the above definitions to private driver/soc/ti/pruss.h?
>>> You can also add pruss_cfg_read and pruss_cfg_update there.
>>>
>
> There is no driver/soc/ti/pruss.h. The pruss.h file is located in
> include/linux/remoteproc/pruss.h and there is one pruss_driver.h file which is
> located in include/linux/pruss_driver.h
>
> Do you want me to create another header file at driver/soc/ti/pruss.h and place
> all these definitions inside that?
>
> Please let me know.

Yes. All private definitions should sit in driver/soc/ti/pruss.h

cheers,
roger

2023-03-16 11:35:21

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 3/5] soc: ti: pruss: Add pruss_cfg_read()/update() API

On 16/03/23 17:02, Roger Quadros wrote:
>
>
> On 16/03/2023 13:29, Md Danish Anwar wrote:
>> Roger,
>>
>> On 16/03/23 16:38, Md Danish Anwar wrote:
>>>
>>> On 15/03/23 17:37, Roger Quadros wrote:
>>>> Danish,
>>>>
>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>> From: Suman Anna <[email protected]>
>>>>>
>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>>> the PRUSS platform driver to read and program respectively a register
>>>>> within the PRUSS CFG sub-module represented by a syscon driver.
>>>>>
>>>>> These APIs are internal to PRUSS driver. Various useful registers
>>>>> and macros for certain register bit-fields and their values have also
>>>>> been added.
>>>>>
>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>> ---
>>>>> drivers/soc/ti/pruss.c | 39 ++++++++++++++
>>>>> include/linux/remoteproc/pruss.h | 87 ++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 126 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>> index c8053c0d735f..26d8129b515c 100644
>>>>> --- a/drivers/soc/ti/pruss.c
>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>> @@ -164,6 +164,45 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>>>
>>>>> +/**
>>>>> + * pruss_cfg_read() - read a PRUSS CFG sub-module register
>>>>> + * @pruss: the pruss instance handle
>>>>> + * @reg: register offset within the CFG sub-module
>>>>> + * @val: pointer to return the value in
>>>>> + *
>>>>> + * Reads a given register within the PRUSS CFG sub-module and
>>>>> + * returns it through the passed-in @val pointer
>>>>> + *
>>>>> + * Return: 0 on success, or an error code otherwise
>>>>> + */
>>>>> +static int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val)
>>>>> +{
>>>>> + if (IS_ERR_OR_NULL(pruss))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + return regmap_read(pruss->cfg_regmap, reg, val);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * pruss_cfg_update() - configure a PRUSS CFG sub-module register
>>>>> + * @pruss: the pruss instance handle
>>>>> + * @reg: register offset within the CFG sub-module
>>>>> + * @mask: bit mask to use for programming the @val
>>>>> + * @val: value to write
>>>>> + *
>>>>> + * Programs a given register within the PRUSS CFG sub-module
>>>>> + *
>>>>> + * Return: 0 on success, or an error code otherwise
>>>>> + */
>>>>> +static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>>> + unsigned int mask, unsigned int val)
>>>>> +{
>>>>> + if (IS_ERR_OR_NULL(pruss))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>>>> +}
>>>>> +
>>>>> static void pruss_of_free_clk_provider(void *data)
>>>>> {
>>>>> struct device_node *clk_mux_np = data;
>>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>>> index 33f930e0a0ce..12ef10b9fe9a 100644
>>>>> --- a/include/linux/remoteproc/pruss.h
>>>>> +++ b/include/linux/remoteproc/pruss.h
>>>>> @@ -10,12 +10,99 @@
>>>>> #ifndef __LINUX_PRUSS_H
>>>>> #define __LINUX_PRUSS_H
>>>>>
>>>>> +#include <linux/bits.h>
>>>>> #include <linux/device.h>
>>>>> #include <linux/err.h>
>>>>> #include <linux/types.h>
>>>>>
>>>>> #define PRU_RPROC_DRVNAME "pru-rproc"
>>>>>
>>>>> +/*
>>>>> + * PRU_ICSS_CFG registers
>>>>> + * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
>>>>> + */
>>>>> +#define PRUSS_CFG_REVID 0x00
>>>>> +#define PRUSS_CFG_SYSCFG 0x04
>>>>> +#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
>>>>> +#define PRUSS_CFG_CGR 0x10
>>>>> +#define PRUSS_CFG_ISRP 0x14
>>>>> +#define PRUSS_CFG_ISP 0x18
>>>>> +#define PRUSS_CFG_IESP 0x1C
>>>>> +#define PRUSS_CFG_IECP 0x20
>>>>> +#define PRUSS_CFG_SCRP 0x24
>>>>> +#define PRUSS_CFG_PMAO 0x28
>>>>> +#define PRUSS_CFG_MII_RT 0x2C
>>>>> +#define PRUSS_CFG_IEPCLK 0x30
>>>>> +#define PRUSS_CFG_SPP 0x34
>>>>> +#define PRUSS_CFG_PIN_MX 0x40
>>>>> +
>>>>> +/* PRUSS_GPCFG register bits */
>>>>> +#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
>>>>> +#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
>>>>> +#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
>>>>> +#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
>>>>> +#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
>>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
>>>>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
>>>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
>>>>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
>>>>> +#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
>>>>> +
>>>>> +#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
>>>>> +#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
>>>>> +
>>>>> +/* PRUSS_MII_RT register bits */
>>>>> +#define PRUSS_MII_RT_EVENT_EN BIT(0)
>>>>> +
>>>>> +/* PRUSS_SPP register bits */
>>>>> +#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>>>>> +#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)
>>>>
>>>> Can we please move all the above definitions to private driver/soc/ti/pruss.h?
>>>> You can also add pruss_cfg_read and pruss_cfg_update there.
>>>>
>>
>> There is no driver/soc/ti/pruss.h. The pruss.h file is located in
>> include/linux/remoteproc/pruss.h and there is one pruss_driver.h file which is
>> located in include/linux/pruss_driver.h
>>
>> Do you want me to create another header file at driver/soc/ti/pruss.h and place
>> all these definitions inside that?
>>
>> Please let me know.
>
> Yes. All private definitions should sit in driver/soc/ti/pruss.h
>

Should I keep the change the name to pruss_internal.h? As we already have a
pruss.h inside include/linux/remoteproc, just to avoid any confusion.

>
> cheers,
> roger

--
Thanks and Regards,
Danish.

2023-03-16 11:37:33

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

Hi,

On 16/03/2023 13:05, Md Danish Anwar wrote:
> Hi Roger,
>
> On 15/03/23 17:52, Roger Quadros wrote:
>>
>>
>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>> From: Suman Anna <[email protected]>
>>>
>>> The PRUSS CFG module is represented as a syscon node and is currently
>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>> pruss_cfg_update() API function.
>>>
>>> Signed-off-by: Suman Anna <[email protected]>
>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>> ---
>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++
>>> 2 files changed, 82 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>> index 26d8129b515c..2f04b7922ddb 100644
>>> --- a/drivers/soc/ti/pruss.c
>>> +++ b/drivers/soc/ti/pruss.c
>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>> }
>>>
>>> +/**
>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
>>> + * @pruss: the pruss instance handle
>>> + * @pru_id: id of the PRU core within the PRUSS
>>> + * @mode: GPI mode to set
>>> + *
>>> + * Sets the GPI mode for a given PRU by programming the
>>> + * corresponding PRUSS_CFG_GPCFGx register
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>> + enum pruss_gpi_mode mode)
>>> +{
>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>> + return -EINVAL;
>>> +
>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>>> + return -EINVAL;
>>> +
>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
>>> +}
>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
>>> +
>>> +/**
>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
>>> + * @pruss: the pruss instance
>>> + * @enable: enable/disable
>>> + *
>>> + * Enable/disable the MII RT Events for the PRUSS.
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>>> +{
>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
>>> +
>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
>>> + PRUSS_MII_RT_EVENT_EN, set);
>>> +}
>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
>>> +
>>> +/**
>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>> + * @pruss: the pruss instance
>>> + * @enable: enable/disable
>>> + * @mask: Mask for PRU / RTU
>>
>> You should not expect the user to provide the mask but only
>> the core type e.g.
>>
>> enum pru_type {
>> PRU_TYPE_PRU = 0,
>> PRU_TYPE_RTU,
>> PRU_TYPE_TX_PRU,
>> PRU_TYPE_MAX,
>> };
>>
>> Then you figure out the mask in the function.
>> Also check for invalid pru_type and return error if so.
>>
>
> Sure Roger, I will create a enum and take it as parameter in API. Based on
> these enum I will calculate mask and do XFR shifting inside the API
> pruss_cfg_xfr_enable().
>
> There are two registers for XFR shift.
>
> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)
>
> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN,
> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU
> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN)
>
> So the enum would be something like this.
>
> /**
> * enum xfr_shift_type - XFR shift type
> * @XFR_SHIFT_PRU: Enables XFR shift for PRU
> * @XFR_SHIFT_RTU: Enables XFR shift for RTU
> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU

This is not required. User can call the API twice. once for PRU and once for RTU.

> * @XFR_SHIFT_MAX: Total number of XFR shift types available.
> *
> */
>
> enum xfr_shift_type {
> XFR_SHIFT_PRU = 0,
> XFR_SHIFT_RTU,
> XFR_SHIFT_PRU_RTU,
> XFR_SHIFT_MAX,
> };

Why do you need this new enum definition?
We already have pru_type defined somewhere. You can move it to a public header
if not there yet.

enum pru_type {
PRU_TYPE_PRU = 0,
PRU_TYPE_RTU,
PRU_TYPE_TX_PRU,
PRU_TYPE_MAX,
};

>
> In pruss_cfg_xfr_enable() API, I will use switch case, and for first three
> enums, I will calculate the mask.
>
> If input is anything other than first three, I will retun -EINVAL. This will
> serve as check for valid xfr_shift_type.
>
> The API will look like this.
>
> int pruss_cfg_xfr_enable(struct pruss *pruss, enum xfr_shift_type xfr_type,
> bool enable);
> {
> u32 mask;
>
> switch (xfr_type) {
> case XFR_SHIFT_PRU:
> mask = PRUSS_SPP_XFER_SHIFT_EN;
> break;
> case XFR_SHIFT_RTU:
> mask = PRUSS_SPP_RTU_XFR_SHIFT_EN;
> break;
> case XFR_SHIFT_PRU_RTU:
> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
> break;
> default:
> return -EINVAL;
> }
>
> u32 set = enable ? mask : 0;
>
> return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
> }
>
> This entire change I will keep as part of this patch only.
>
> Please let me know if this looks OK to you.
>
>

cheers,
-roger

2023-03-16 11:45:29

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 16/03/23 17:06, Roger Quadros wrote:
> Hi,
>
> On 16/03/2023 13:05, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 15/03/23 17:52, Roger Quadros wrote:
>>>
>>>
>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>> From: Suman Anna <[email protected]>
>>>>
>>>> The PRUSS CFG module is represented as a syscon node and is currently
>>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>>> pruss_cfg_update() API function.
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>> ---
>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++
>>>> 2 files changed, 82 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index 26d8129b515c..2f04b7922ddb 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>>> }
>>>>
>>>> +/**
>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
>>>> + * @pruss: the pruss instance handle
>>>> + * @pru_id: id of the PRU core within the PRUSS
>>>> + * @mode: GPI mode to set
>>>> + *
>>>> + * Sets the GPI mode for a given PRU by programming the
>>>> + * corresponding PRUSS_CFG_GPCFGx register
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise
>>>> + */
>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>>> + enum pruss_gpi_mode mode)
>>>> +{
>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>>> + return -EINVAL;
>>>> +
>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>>>> + return -EINVAL;
>>>> +
>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
>>>> +
>>>> +/**
>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
>>>> + * @pruss: the pruss instance
>>>> + * @enable: enable/disable
>>>> + *
>>>> + * Enable/disable the MII RT Events for the PRUSS.
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise
>>>> + */
>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>>>> +{
>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
>>>> +
>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
>>>> + PRUSS_MII_RT_EVENT_EN, set);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
>>>> +
>>>> +/**
>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>>> + * @pruss: the pruss instance
>>>> + * @enable: enable/disable
>>>> + * @mask: Mask for PRU / RTU
>>>
>>> You should not expect the user to provide the mask but only
>>> the core type e.g.
>>>
>>> enum pru_type {
>>> PRU_TYPE_PRU = 0,
>>> PRU_TYPE_RTU,
>>> PRU_TYPE_TX_PRU,
>>> PRU_TYPE_MAX,
>>> };
>>>
>>> Then you figure out the mask in the function.
>>> Also check for invalid pru_type and return error if so.
>>>
>>
>> Sure Roger, I will create a enum and take it as parameter in API. Based on
>> these enum I will calculate mask and do XFR shifting inside the API
>> pruss_cfg_xfr_enable().
>>
>> There are two registers for XFR shift.
>>
>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)
>>
>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN,
>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU
>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN)
>>
>> So the enum would be something like this.
>>
>> /**
>> * enum xfr_shift_type - XFR shift type
>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU
>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU
>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU
>
> This is not required. User can call the API twice. once for PRU and once for RTU.
>
>> * @XFR_SHIFT_MAX: Total number of XFR shift types available.
>> *
>> */
>>
>> enum xfr_shift_type {
>> XFR_SHIFT_PRU = 0,
>> XFR_SHIFT_RTU,
>> XFR_SHIFT_PRU_RTU,
>> XFR_SHIFT_MAX,
>> };
>
> Why do you need this new enum definition?
> We already have pru_type defined somewhere. You can move it to a public header
> if not there yet.
>
> enum pru_type {
> PRU_TYPE_PRU = 0,
> PRU_TYPE_RTU,
> PRU_TYPE_TX_PRU,
> PRU_TYPE_MAX,
> };
>

This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem
with this enum is that in [1] we need to enable XFR shift for both PRU and RTU
for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and
PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU).

Now this enum doesn't have a field for both PRU and RTU. Also we don't need
need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are
defined.

That is why I thought of introducing new enum.

[1] drivers/net/ethernet/ti/icssg_config.c

/* enable XFR shift for PRU and RTU */
mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
>
>>
>> In pruss_cfg_xfr_enable() API, I will use switch case, and for first three
>> enums, I will calculate the mask.
>>
>> If input is anything other than first three, I will retun -EINVAL. This will
>> serve as check for valid xfr_shift_type.
>>
>> The API will look like this.
>>
>> int pruss_cfg_xfr_enable(struct pruss *pruss, enum xfr_shift_type xfr_type,
>> bool enable);
>> {
>> u32 mask;
>>
>> switch (xfr_type) {
>> case XFR_SHIFT_PRU:
>> mask = PRUSS_SPP_XFER_SHIFT_EN;
>> break;
>> case XFR_SHIFT_RTU:
>> mask = PRUSS_SPP_RTU_XFR_SHIFT_EN;
>> break;
>> case XFR_SHIFT_PRU_RTU:
>> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
>> break;
>> default:
>> return -EINVAL;
>> }
>>
>> u32 set = enable ? mask : 0;
>>
>> return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
>> }
>>
>> This entire change I will keep as part of this patch only.
>>
>> Please let me know if this looks OK to you.
>>
>>
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-16 12:19:25

[permalink] [raw]

Subject: Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 16/03/2023 13:44, Md Danish Anwar wrote:
>
> On 16/03/23 17:06, Roger Quadros wrote:
>> Hi,
>>
>> On 16/03/2023 13:05, Md Danish Anwar wrote:
>>> Hi Roger,
>>>
>>> On 15/03/23 17:52, Roger Quadros wrote:
>>>>
>>>>
>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>> From: Suman Anna <[email protected]>
>>>>>
>>>>> The PRUSS CFG module is represented as a syscon node and is currently
>>>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>>>> pruss_cfg_update() API function.
>>>>>
>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>> ---
>>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
>>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++
>>>>> 2 files changed, 82 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>> index 26d8129b515c..2f04b7922ddb 100644
>>>>> --- a/drivers/soc/ti/pruss.c
>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
>>>>> + * @pruss: the pruss instance handle
>>>>> + * @pru_id: id of the PRU core within the PRUSS
>>>>> + * @mode: GPI mode to set
>>>>> + *
>>>>> + * Sets the GPI mode for a given PRU by programming the
>>>>> + * corresponding PRUSS_CFG_GPCFGx register
>>>>> + *
>>>>> + * Return: 0 on success, or an error code otherwise
>>>>> + */
>>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>>>> + enum pruss_gpi_mode mode)
>>>>> +{
>>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
>>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
>>>>> +
>>>>> +/**
>>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
>>>>> + * @pruss: the pruss instance
>>>>> + * @enable: enable/disable
>>>>> + *
>>>>> + * Enable/disable the MII RT Events for the PRUSS.
>>>>> + *
>>>>> + * Return: 0 on success, or an error code otherwise
>>>>> + */
>>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>>>>> +{
>>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
>>>>> +
>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
>>>>> + PRUSS_MII_RT_EVENT_EN, set);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
>>>>> +
>>>>> +/**
>>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>>>> + * @pruss: the pruss instance
>>>>> + * @enable: enable/disable
>>>>> + * @mask: Mask for PRU / RTU
>>>>
>>>> You should not expect the user to provide the mask but only
>>>> the core type e.g.
>>>>
>>>> enum pru_type {
>>>> PRU_TYPE_PRU = 0,
>>>> PRU_TYPE_RTU,
>>>> PRU_TYPE_TX_PRU,
>>>> PRU_TYPE_MAX,
>>>> };
>>>>
>>>> Then you figure out the mask in the function.
>>>> Also check for invalid pru_type and return error if so.
>>>>
>>>
>>> Sure Roger, I will create a enum and take it as parameter in API. Based on
>>> these enum I will calculate mask and do XFR shifting inside the API
>>> pruss_cfg_xfr_enable().
>>>
>>> There are two registers for XFR shift.
>>>
>>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)
>>>
>>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN,
>>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU
>>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN)
>>>
>>> So the enum would be something like this.
>>>
>>> /**
>>> * enum xfr_shift_type - XFR shift type
>>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU
>>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU
>>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU
>>
>> This is not required. User can call the API twice. once for PRU and once for RTU.
>>
>>> * @XFR_SHIFT_MAX: Total number of XFR shift types available.
>>> *
>>> */
>>>
>>> enum xfr_shift_type {
>>> XFR_SHIFT_PRU = 0,
>>> XFR_SHIFT_RTU,
>>> XFR_SHIFT_PRU_RTU,
>>> XFR_SHIFT_MAX,
>>> };
>>
>> Why do you need this new enum definition?
>> We already have pru_type defined somewhere. You can move it to a public header
>> if not there yet.
>>
>> enum pru_type {
>> PRU_TYPE_PRU = 0,
>> PRU_TYPE_RTU,
>> PRU_TYPE_TX_PRU,
>> PRU_TYPE_MAX,
>> };
>>
>
> This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem
> with this enum is that in [1] we need to enable XFR shift for both PRU and RTU
> for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and
> PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU).

Is there any limitation that you have to enable both simultaneously?
The driver can first do enable for PRU and then later for RTU.

As you will do a read modify write, the previous enable state of register
shouldn't be affected.

>
> Now this enum doesn't have a field for both PRU and RTU. Also we don't need
> need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are
> defined.

That is OK. You can return error if not RTU or PRU.

>
> That is why I thought of introducing new enum.
>
> [1] drivers/net/ethernet/ti/icssg_config.c
>
> /* enable XFR shift for PRU and RTU */
> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;

Driver can do like so

pruss_cfg_xfr_enable(pruss, PRU_TYPE_PRU, true);
pruss_cfg_xfr_enable(pruss, PRU_TYPE_RTU, true);

The second call should not disable the PRU XFR as you will do a
read-modify-write only affecting the RTU bit.

cheers,
-roger

2023-03-16 13:11:56

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 16/03/23 17:49, Roger Quadros wrote:
>
>
> On 16/03/2023 13:44, Md Danish Anwar wrote:
>>
>> On 16/03/23 17:06, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 16/03/2023 13:05, Md Danish Anwar wrote:
>>>> Hi Roger,
>>>>
>>>> On 15/03/23 17:52, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>>> From: Suman Anna <[email protected]>
>>>>>>
>>>>>> The PRUSS CFG module is represented as a syscon node and is currently
>>>>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>>>>> pruss_cfg_update() API function.
>>>>>>
>>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>>> ---
>>>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
>>>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++
>>>>>> 2 files changed, 82 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>>> index 26d8129b515c..2f04b7922ddb 100644
>>>>>> --- a/drivers/soc/ti/pruss.c
>>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>>>>> }
>>>>>>
>>>>>> +/**
>>>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
>>>>>> + * @pruss: the pruss instance handle
>>>>>> + * @pru_id: id of the PRU core within the PRUSS
>>>>>> + * @mode: GPI mode to set
>>>>>> + *
>>>>>> + * Sets the GPI mode for a given PRU by programming the
>>>>>> + * corresponding PRUSS_CFG_GPCFGx register
>>>>>> + *
>>>>>> + * Return: 0 on success, or an error code otherwise
>>>>>> + */
>>>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>>>>> + enum pruss_gpi_mode mode)
>>>>>> +{
>>>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>>>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
>>>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
>>>>>> +
>>>>>> +/**
>>>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
>>>>>> + * @pruss: the pruss instance
>>>>>> + * @enable: enable/disable
>>>>>> + *
>>>>>> + * Enable/disable the MII RT Events for the PRUSS.
>>>>>> + *
>>>>>> + * Return: 0 on success, or an error code otherwise
>>>>>> + */
>>>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>>>>>> +{
>>>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
>>>>>> +
>>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
>>>>>> + PRUSS_MII_RT_EVENT_EN, set);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
>>>>>> +
>>>>>> +/**
>>>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>>>>> + * @pruss: the pruss instance
>>>>>> + * @enable: enable/disable
>>>>>> + * @mask: Mask for PRU / RTU
>>>>>
>>>>> You should not expect the user to provide the mask but only
>>>>> the core type e.g.
>>>>>
>>>>> enum pru_type {
>>>>> PRU_TYPE_PRU = 0,
>>>>> PRU_TYPE_RTU,
>>>>> PRU_TYPE_TX_PRU,
>>>>> PRU_TYPE_MAX,
>>>>> };
>>>>>
>>>>> Then you figure out the mask in the function.
>>>>> Also check for invalid pru_type and return error if so.
>>>>>
>>>>
>>>> Sure Roger, I will create a enum and take it as parameter in API. Based on
>>>> these enum I will calculate mask and do XFR shifting inside the API
>>>> pruss_cfg_xfr_enable().
>>>>
>>>> There are two registers for XFR shift.
>>>>
>>>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>>>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)
>>>>
>>>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN,
>>>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU
>>>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN)
>>>>
>>>> So the enum would be something like this.
>>>>
>>>> /**
>>>> * enum xfr_shift_type - XFR shift type
>>>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU
>>>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU
>>>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU
>>>
>>> This is not required. User can call the API twice. once for PRU and once for RTU.
>>>
>>>> * @XFR_SHIFT_MAX: Total number of XFR shift types available.
>>>> *
>>>> */
>>>>
>>>> enum xfr_shift_type {
>>>> XFR_SHIFT_PRU = 0,
>>>> XFR_SHIFT_RTU,
>>>> XFR_SHIFT_PRU_RTU,
>>>> XFR_SHIFT_MAX,
>>>> };
>>>
>>> Why do you need this new enum definition?
>>> We already have pru_type defined somewhere. You can move it to a public header
>>> if not there yet.
>>>
>>> enum pru_type {
>>> PRU_TYPE_PRU = 0,
>>> PRU_TYPE_RTU,
>>> PRU_TYPE_TX_PRU,
>>> PRU_TYPE_MAX,
>>> };
>>>
>>
>> This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem
>> with this enum is that in [1] we need to enable XFR shift for both PRU and RTU
>> for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and
>> PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU).
>
> Is there any limitation that you have to enable both simultaneously?
> The driver can first do enable for PRU and then later for RTU.
>
> As you will do a read modify write, the previous enable state of register
> shouldn't be affected.
>
>>
>> Now this enum doesn't have a field for both PRU and RTU. Also we don't need
>> need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are
>> defined.
>
> That is OK. You can return error if not RTU or PRU.
>
>>
>> That is why I thought of introducing new enum.
>>
>> [1] drivers/net/ethernet/ti/icssg_config.c
>>
>> /* enable XFR shift for PRU and RTU */
>> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
>
> Driver can do like so
>
> pruss_cfg_xfr_enable(pruss, PRU_TYPE_PRU, true);
> pruss_cfg_xfr_enable(pruss, PRU_TYPE_RTU, true);
>
> The second call should not disable the PRU XFR as you will do a
> read-modify-write only affecting the RTU bit.

Sure, then I will use the existing enum pru_type.

The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this
enum definition from there to include/linux/remoteproc/pruss.h

>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-16 14:04:32

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

Hi,

On 16/03/2023 15:11, Md Danish Anwar wrote:
>
>
> On 16/03/23 17:49, Roger Quadros wrote:
>>
>>
>> On 16/03/2023 13:44, Md Danish Anwar wrote:
>>>
>>> On 16/03/23 17:06, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 16/03/2023 13:05, Md Danish Anwar wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 15/03/23 17:52, Roger Quadros wrote:
>>>>>>
>>>>>>
>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>>>> From: Suman Anna <[email protected]>
>>>>>>>
>>>>>>> The PRUSS CFG module is represented as a syscon node and is currently
>>>>>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>>>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>>>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>>>>>> pruss_cfg_update() API function.
>>>>>>>
>>>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>>>> ---
>>>>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++
>>>>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++
>>>>>>> 2 files changed, 82 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>>>> index 26d8129b515c..2f04b7922ddb 100644
>>>>>>> --- a/drivers/soc/ti/pruss.c
>>>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>>>>>> }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU
>>>>>>> + * @pruss: the pruss instance handle
>>>>>>> + * @pru_id: id of the PRU core within the PRUSS
>>>>>>> + * @mode: GPI mode to set
>>>>>>> + *
>>>>>>> + * Sets the GPI mode for a given PRU by programming the
>>>>>>> + * corresponding PRUSS_CFG_GPCFGx register
>>>>>>> + *
>>>>>>> + * Return: 0 on success, or an error code otherwise
>>>>>>> + */
>>>>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>>>>>> + enum pruss_gpi_mode mode)
>>>>>>> +{
>>>>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>>>>>>> + return -EINVAL;
>>>>>>> +
>>>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>>>>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
>>>>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events
>>>>>>> + * @pruss: the pruss instance
>>>>>>> + * @enable: enable/disable
>>>>>>> + *
>>>>>>> + * Enable/disable the MII RT Events for the PRUSS.
>>>>>>> + *
>>>>>>> + * Return: 0 on success, or an error code otherwise
>>>>>>> + */
>>>>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable)
>>>>>>> +{
>>>>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0;
>>>>>>> +
>>>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT,
>>>>>>> + PRUSS_MII_RT_EVENT_EN, set);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>>>>>> + * @pruss: the pruss instance
>>>>>>> + * @enable: enable/disable
>>>>>>> + * @mask: Mask for PRU / RTU
>>>>>>
>>>>>> You should not expect the user to provide the mask but only
>>>>>> the core type e.g.
>>>>>>
>>>>>> enum pru_type {
>>>>>> PRU_TYPE_PRU = 0,
>>>>>> PRU_TYPE_RTU,
>>>>>> PRU_TYPE_TX_PRU,
>>>>>> PRU_TYPE_MAX,
>>>>>> };
>>>>>>
>>>>>> Then you figure out the mask in the function.
>>>>>> Also check for invalid pru_type and return error if so.
>>>>>>
>>>>>
>>>>> Sure Roger, I will create a enum and take it as parameter in API. Based on
>>>>> these enum I will calculate mask and do XFR shifting inside the API
>>>>> pruss_cfg_xfr_enable().
>>>>>
>>>>> There are two registers for XFR shift.
>>>>>
>>>>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>>>>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)
>>>>>
>>>>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN,
>>>>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU
>>>>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN)
>>>>>
>>>>> So the enum would be something like this.
>>>>>
>>>>> /**
>>>>> * enum xfr_shift_type - XFR shift type
>>>>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU
>>>>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU
>>>>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU
>>>>
>>>> This is not required. User can call the API twice. once for PRU and once for RTU.
>>>>
>>>>> * @XFR_SHIFT_MAX: Total number of XFR shift types available.
>>>>> *
>>>>> */
>>>>>
>>>>> enum xfr_shift_type {
>>>>> XFR_SHIFT_PRU = 0,
>>>>> XFR_SHIFT_RTU,
>>>>> XFR_SHIFT_PRU_RTU,
>>>>> XFR_SHIFT_MAX,
>>>>> };
>>>>
>>>> Why do you need this new enum definition?
>>>> We already have pru_type defined somewhere. You can move it to a public header
>>>> if not there yet.
>>>>
>>>> enum pru_type {
>>>> PRU_TYPE_PRU = 0,
>>>> PRU_TYPE_RTU,
>>>> PRU_TYPE_TX_PRU,
>>>> PRU_TYPE_MAX,
>>>> };
>>>>
>>>
>>> This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem
>>> with this enum is that in [1] we need to enable XFR shift for both PRU and RTU
>>> for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and
>>> PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU).
>>
>> Is there any limitation that you have to enable both simultaneously?
>> The driver can first do enable for PRU and then later for RTU.
>>
>> As you will do a read modify write, the previous enable state of register
>> shouldn't be affected.
>>
>>>
>>> Now this enum doesn't have a field for both PRU and RTU. Also we don't need
>>> need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are
>>> defined.
>>
>> That is OK. You can return error if not RTU or PRU.
>>
>>>
>>> That is why I thought of introducing new enum.
>>>
>>> [1] drivers/net/ethernet/ti/icssg_config.c
>>>
>>> /* enable XFR shift for PRU and RTU */
>>> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
>>
>> Driver can do like so
>>
>> pruss_cfg_xfr_enable(pruss, PRU_TYPE_PRU, true);
>> pruss_cfg_xfr_enable(pruss, PRU_TYPE_RTU, true);
>>
>> The second call should not disable the PRU XFR as you will do a
>> read-modify-write only affecting the RTU bit.
>
> Sure, then I will use the existing enum pru_type.
>
> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this
> enum definition from there to include/linux/remoteproc/pruss.h

There are 2 public pruss.h files.
include/linux/remoteproc/pruss.h
and
include/linux/pruss_driver.h

Why is that and when to use what?

cheers,
-roger

2023-03-17 05:03:03

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 16/03/23 19:34, Roger Quadros wrote:
>
> Hi,
>
> On 16/03/2023 15:11, Md Danish Anwar wrote:
>>
>>
>> On 16/03/23 17:49, Roger Quadros wrote:
>>>
>>>
>>> On 16/03/2023 13:44, Md Danish Anwar wrote:
>>>>
>>>> On 16/03/23 17:06, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 16/03/2023 13:05, Md Danish Anwar wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On 15/03/23 17:52, Roger Quadros wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>>>>> From: Suman Anna <[email protected]>
>

[..]

>> Sure, then I will use the existing enum pru_type.
>>
>> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this
>> enum definition from there to include/linux/remoteproc/pruss.h
>
> There are 2 public pruss.h files.
> include/linux/remoteproc/pruss.h
> and
> include/linux/pruss_driver.h
>
> Why is that and when to use what?
>

The include/linux/remoteproc/pruss.h file was introduced in series [1] as a
public header file for PRU_RPROC driver (drivers/remoteproc/pru_rproc.c)

The second header file include/linux/pruss_driver.h was introduced much earlier
as part of [2] , "soc: ti: pruss: Add a platform driver for PRUSS in TI SoCs".

As far as I can see, seems like pruss_driver.h was added as a public header
file for PRUSS platform driver (drivers/soc/ti/pruss.c)

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/

> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-17 08:33:46

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 17/03/2023 07:02, Md Danish Anwar wrote:
>
>
> On 16/03/23 19:34, Roger Quadros wrote:
>>
>> Hi,
>>
>> On 16/03/2023 15:11, Md Danish Anwar wrote:
>>>
>>>
>>> On 16/03/23 17:49, Roger Quadros wrote:
>>>>
>>>>
>>>> On 16/03/2023 13:44, Md Danish Anwar wrote:
>>>>>
>>>>> On 16/03/23 17:06, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 16/03/2023 13:05, Md Danish Anwar wrote:
>>>>>>> Hi Roger,
>>>>>>>
>>>>>>> On 15/03/23 17:52, Roger Quadros wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>>>>>> From: Suman Anna <[email protected]>
>>
>
> [..]
>
>>> Sure, then I will use the existing enum pru_type.
>>>
>>> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this
>>> enum definition from there to include/linux/remoteproc/pruss.h
>>
>> There are 2 public pruss.h files.
>> include/linux/remoteproc/pruss.h
>> and
>> include/linux/pruss_driver.h
>>
>> Why is that and when to use what?
>>
>
> The include/linux/remoteproc/pruss.h file was introduced in series [1] as a
> public header file for PRU_RPROC driver (drivers/remoteproc/pru_rproc.c)
>
> The second header file include/linux/pruss_driver.h was introduced much earlier
> as part of [2] , "soc: ti: pruss: Add a platform driver for PRUSS in TI SoCs".
>
> As far as I can see, seems like pruss_driver.h was added as a public header
> file for PRUSS platform driver (drivers/soc/ti/pruss.c)
>
> [1] https://lore.kernel.org/all/[email protected]/
> [2] https://lore.kernel.org/all/[email protected]/

Thanks. "include/linux/remoteproc/pruss.h" seems appropriate for enum pru_type.

cheers,
-roger

2023-03-17 08:55:52

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 4/5] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR

On 17/03/23 14:01, Roger Quadros wrote:
>
>
> On 17/03/2023 07:02, Md Danish Anwar wrote:
>>
>>
>> On 16/03/23 19:34, Roger Quadros wrote:
>>>
>>> Hi,
>>>
>>> On 16/03/2023 15:11, Md Danish Anwar wrote:
>>>>
>>>>
>>>> On 16/03/23 17:49, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 16/03/2023 13:44, Md Danish Anwar wrote:
>>>>>>
>>>>>> On 16/03/23 17:06, Roger Quadros wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 16/03/2023 13:05, Md Danish Anwar wrote:
>>>>>>>> Hi Roger,
>>>>>>>>
>>>>>>>> On 15/03/23 17:52, Roger Quadros wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote:
>>>>>>>>>> From: Suman Anna <[email protected]>
>>>
>>
>> [..]
>>
>>>> Sure, then I will use the existing enum pru_type.
>>>>
>>>> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this
>>>> enum definition from there to include/linux/remoteproc/pruss.h
>>>
>>> There are 2 public pruss.h files.
>>> include/linux/remoteproc/pruss.h
>>> and
>>> include/linux/pruss_driver.h
>>>
>>> Why is that and when to use what?
>>>
>>
>> The include/linux/remoteproc/pruss.h file was introduced in series [1] as a
>> public header file for PRU_RPROC driver (drivers/remoteproc/pru_rproc.c)
>>
>> The second header file include/linux/pruss_driver.h was introduced much earlier
>> as part of [2] , "soc: ti: pruss: Add a platform driver for PRUSS in TI SoCs".
>>
>> As far as I can see, seems like pruss_driver.h was added as a public header
>> file for PRUSS platform driver (drivers/soc/ti/pruss.c)
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>> [2] https://lore.kernel.org/all/[email protected]/
>
> Thanks. "include/linux/remoteproc/pruss.h" seems appropriate for enum pru_type.
>
> cheers,
> -roger

Yes, enum pru_type is located in pru_rproc.c, I will move enum pru_type to
include/linux/remoteproc/pruss.h and then include
include/linux/remoteproc/pruss.h" in pru_rproc.c and any other file that needs
this enum.

--
Thanks and Regards,
Danish.

2023-03-17 08:56:52

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

Hi Andrew & Danish,

On 13/03/2023 13:11, MD Danish Anwar wrote:
> From: "Andrew F. Davis" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Suman Anna <[email protected]>
> Signed-off-by: Andrew F. Davis <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: MD Danish Anwar <[email protected]>
> Reviewed-by: Roger Quadros <[email protected]>
> ---
> 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

2023-03-20 05:12:04

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

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" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Suman Anna <[email protected]>
>> Signed-off-by: Andrew F. Davis <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: MD Danish Anwar <[email protected]>
>> Reviewed-by: Roger Quadros <[email protected]>
>> ---
>> 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

--
Thanks and Regards,
Danish.

2023-03-20 16:27:15

by Andrew Davis

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

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" <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Suman Anna <[email protected]>
>>> Signed-off-by: Andrew F. Davis <[email protected]>
>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>> Reviewed-by: Roger Quadros <[email protected]>
>>> ---
>>> 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
>

2023-03-21 05:24:33

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

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" <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Signed-off-by: Andrew F. Davis <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>> Reviewed-by: Roger Quadros <[email protected]>
>>>> ---
>>>> 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
>>

--
Thanks and Regards,
Danish.

2023-03-21 09:24:57

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

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" <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>> Signed-off-by: Andrew F. Davis <[email protected]>
>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>> Reviewed-by: Roger Quadros <[email protected]>
>>>>> ---
>>>>> 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

2023-03-21 09:49:15

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

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" <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>>> Signed-off-by: Andrew F. Davis <[email protected]>
>>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>>> Reviewed-by: Roger Quadros <[email protected]>
>>>>>> ---
>>>>>> 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

--
Thanks and Regards,
Danish.

2023-03-21 10:24:02

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/5] soc: ti: pruss: Add pruss_{request,release}_mem_region() API

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" <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>>>> Signed-off-by: Andrew F. Davis <[email protected]>
>>>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>>>>> Reviewed-by: Roger Quadros <[email protected]>
>>>>>>> ---
>>>>>>> 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

2023-03-21 10:46:17