[PATCH v5 0/5] Introduce PRU platform consumer API

[PATCH v5 0/5] Introduce PRU platform consumer API

[MD Danish Anwar]

Hi All,
The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS
or simply PRUSS) on various TI SoCs consists of dual 32-bit RISC cores
(Programmable Real-Time Units, or PRUs) for program execution.

There are 3 foundation components for TI PRUSS subsystem: the PRUSS platform
driver, the PRUSS INTC driver and the PRUSS remoteproc driver. All of them have
already been merged and can be found under:
1) drivers/soc/ti/pruss.c
   Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
2) drivers/irqchip/irq-pruss-intc.c
   Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml
3) drivers/remoteproc/pru_rproc.c
   Documentation/devicetree/bindings/remoteproc/ti,pru-consumer.yaml

The programmable nature of the PRUs provide flexibility to implement custom
peripheral interfaces, fast real-time responses, or specialized data handling.
Example of a PRU consumer drivers will be: 
  - Software UART over PRUSS
  - PRU-ICSS Ethernet EMAC

In order to make usage of common PRU resources and allow the consumer drivers 
to configure the PRU hardware for specific usage the PRU API is introduced.

This is the v5 of the old patch series[1]. This doesn't have any functional 
changes, the old series has been rebased on linux-next.

Changes from v4 [1] to v5:
*) Addressed Roger's comment to change function argument in API 
pruss_cfg_xfr_enable(). Instead of asking user to calcualte mask, now user
will just provide the pru_type and mask will be calcualted inside the API.
*) Moved enum pru_type from pru_rproc.c to include/linux/remoteproc/pruss.h
in patch 4 / 5.
*) Moved enum pruss_gpi_mode from patch 3/5 to patch 4/5 to introduce this
enum in same patch as the API using it.
*) Moved enum pruss_gp_mux_sel from patch 3/5 to patch 5/5 to introduce this
enum in same patch as the API using it.
*) Created new headefile drivers/soc/ti/pruss.h, private to PRUSS as asked by
Roger. Moved all private definitions and pruss_cfg_read () / update ()
APIs to this newly added headerfile.
*) Renamed include/linux/pruss_driver.h to include/linux/pruss_internal.h as
suggested by Andrew and Roger.

Changes from v3 [2] to v4:
*) Added my SoB tags in all patches as earlier SoB tags were missing in few
patches.
*) Added Roger's RB tags in 3 patches.
*) Addressed Roger's comment in patch 4/5 of this series. Added check for 
   invalid GPI mode in pruss_cfg_gpimode() API.
*) Removed patch [5] from this series as that patch is no longer required.
*) Made pruss_cfg_read() and pruss_cfg_update() APIs internal to pruss.c by
   removing EXPORT_SYMBOL_GPL and making them static. Now these APIs are 
   internal to pruss.c and PRUSS CFG space is not exposed.
*) Moved APIs pruss_cfg_gpimode(), pruss_cfg_miirt_enable(), 
   pruss_cfg_xfr_enable(), pruss_cfg_get_gpmux(), pruss_cfg_set_gpmux() to
   pruss.c file as they are using APIs pruss_cfg_read / update. 
   Defined these APIs in pruss.h file as other drivers use these APIs to 
   perform respective operations.

Changes from v2 to v3:
*) No functional changes, the old series has been rebased on linux-next (tag:
next-20230306).

This series depends on another series which is already merged in the remoteproc
tree [3] and is part of v6.3-rc1. This series and the remoteproc series form 
the PRUSS consumer API which can be used by consumer drivers to utilize the 
PRUs.

One example of the consumer driver is the PRU-ICSSG ethernet driver [4],which 
depends on this series and the remoteproc series [3].

[1] https://lore.kernel.org/all/20230313111127.1229187-1-danishanwar@ti.com/
[2] https://lore.kernel.org/all/20230306110934.2736465-1-danishanwar@ti.com/
[3] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/#t
[4] https://lore.kernel.org/all/20230210114957.2667963-1-danishanwar@ti.com/
[5] https://lore.kernel.org/all/20230306110934.2736465-6-danishanwar@ti.com/

Thanks and Regards,
Md Danish Anwar

Andrew F. Davis (1):
  soc: ti: pruss: Add pruss_{request,release}_mem_region() API

Suman Anna (2):
  soc: ti: pruss: Add pruss_cfg_read()/update() API
  soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and
    XFR

Tero Kristo (2):
  soc: ti: pruss: Add pruss_get()/put() API
  soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

 drivers/remoteproc/pru_rproc.c                |  17 +-
 drivers/soc/ti/pruss.c                        | 256 +++++++++++++++++-
 drivers/soc/ti/pruss.h                        | 112 ++++++++
 .../{pruss_driver.h => pruss_internal.h}      |  34 +--
 include/linux/remoteproc/pruss.h              | 139 ++++++++++
 5 files changed, 516 insertions(+), 42 deletions(-)
 create mode 100644 drivers/soc/ti/pruss.h
 rename include/linux/{pruss_driver.h => pruss_internal.h} (58%)

-- 
2.25.1

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

[MD Danish Anwar]

From: Tero Kristo <t-kristo@ti.com>

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 <s-anna@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/remoteproc/pru_rproc.c                |  2 +-
 drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
 .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
 include/linux/remoteproc/pruss.h              | 19 ++++++
 4 files changed, 83 insertions(+), 5 deletions(-)
 rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index b76db7fa693d..4ddd5854d56e 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -19,7 +19,7 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/remoteproc/pruss.h>
-#include <linux/pruss_driver.h>
+#include <linux/pruss_internal.h>
 #include <linux/remoteproc.h>
 
 #include "remoteproc_internal.h"
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 6882c86b3ce5..6c2bb02a521d 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -6,6 +6,7 @@
  * Author(s):
  *	Suman Anna <s-anna@ti.com>
  *	Andrew F. Davis <afd@ti.com>
+ *	Tero Kristo <t-kristo@ti.com>
  */
 
 #include <linux/clk-provider.h>
@@ -16,8 +17,9 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
-#include <linux/pruss_driver.h>
+#include <linux/pruss_internal.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_internal.h
similarity index 90%
rename from include/linux/pruss_driver.h
rename to include/linux/pruss_internal.h
index ecfded30ed05..8f91cb164054 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_internal.h
@@ -6,9 +6,10 @@
  *	Suman Anna <s-anna@ti.com>
  */
 
-#ifndef _PRUSS_DRIVER_H_
-#define _PRUSS_DRIVER_H_
+#ifndef _PRUSS_INTERNAL_H_
+#define _PRUSS_INTERNAL_H_
 
+#include <linux/remoteproc/pruss.h>
 #include <linux/types.h>
 
 /*
@@ -51,4 +52,4 @@ struct pruss {
 	struct clk *iep_clk_mux;
 };
 
-#endif	/* _PRUSS_DRIVER_H_ */
+#endif	/* _PRUSS_INTERNAL_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 <s-anna@ti.com>
+ *	Tero Kristo <t-kristo@ti.com>
  */
 
 #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

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

[MD Danish Anwar]

From: "Andrew F. Davis" <afd@ti.com>

Add two new API - pruss_request_mem_region() & pruss_release_mem_region(),
to the PRUSS platform driver to allow client drivers to acquire and release
the common memory resources present within a PRU-ICSS subsystem. This
allows the client drivers to directly manipulate the respective memories,
as per their design contract with the associated firmware.

Co-developed-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Andrew F. Davis <afd@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/soc/ti/pruss.c           | 77 ++++++++++++++++++++++++++++++++
 include/linux/pruss_internal.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 6c2bb02a521d..126b672b9b30 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_internal.h b/include/linux/pruss_internal.h
index 8f91cb164054..cf5287fa01df 100644
--- a/include/linux/pruss_internal.h
+++ b/include/linux/pruss_internal.h
@@ -9,37 +9,18 @@
 #ifndef _PRUSS_INTERNAL_H_
 #define _PRUSS_INTERNAL_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

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

[MD Danish Anwar]

From: Suman Anna <s-anna@ti.com>

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 <s-anna@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/soc/ti/pruss.c |   1 +
 drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 drivers/soc/ti/pruss.h

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 126b672b9b30..2fa7df667592 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -21,6 +21,7 @@
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
 #include <linux/slab.h>
+#include "pruss.h"
 
 /**
  * struct pruss_private_data - PRUSS driver private data
diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
new file mode 100644
index 000000000000..4626d5f6b874
--- /dev/null
+++ b/drivers/soc/ti/pruss.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * PRU-ICSS Subsystem user interfaces
+ *
+ * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
+ *	MD Danish Anwar <danishanwar@ti.com>
+ */
+
+#ifndef _SOC_TI_PRUSS_H_
+#define _SOC_TI_PRUSS_H_
+
+#include <linux/bits.h>
+#include <linux/regmap.h>
+
+/*
+ * 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)
+#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
+
+/**
+ * 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);
+}
+
+#endif  /* _SOC_TI_PRUSS_H_ */
-- 
2.25.1

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

[MD Danish Anwar]

From: Suman Anna <s-anna@ti.com>

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 <s-anna@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/remoteproc/pru_rproc.c   | 15 -------
 drivers/soc/ti/pruss.c           | 74 ++++++++++++++++++++++++++++++++
 include/linux/remoteproc/pruss.h | 51 ++++++++++++++++++++++
 3 files changed, 125 insertions(+), 15 deletions(-)

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index 4ddd5854d56e..a88861737dec 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -81,21 +81,6 @@ enum pru_iomem {
 	PRU_IOMEM_MAX,
 };
 
-/**
- * enum pru_type - PRU core type identifier
- *
- * @PRU_TYPE_PRU: Programmable Real-time Unit
- * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
- * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
- * @PRU_TYPE_MAX: just keep this one at the end
- */
-enum pru_type {
-	PRU_TYPE_PRU = 0,
-	PRU_TYPE_RTU,
-	PRU_TYPE_TX_PRU,
-	PRU_TYPE_MAX,
-};
-
 /**
  * struct pru_private_data - device data for a PRU core
  * @type: type of the PRU core (PRU, RTU, Tx_PRU)
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 2fa7df667592..ac415442e85b 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -165,6 +165,80 @@ int pruss_release_mem_region(struct pruss *pruss,
 }
 EXPORT_SYMBOL_GPL(pruss_release_mem_region);
 
+/**
+ * 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
+ * @pru_type: PRU core type identifier
+ * @enable: enable/disable
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
+			 bool enable)
+{
+	u32 mask, set;
+
+	switch (pru_type) {
+	case PRU_TYPE_PRU:
+		mask = PRUSS_SPP_XFER_SHIFT_EN;
+		break;
+	case PRU_TYPE_RTU:
+		mask = PRUSS_SPP_RTU_XFR_SHIFT_EN;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	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 33f930e0a0ce..bb001f712980 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -16,6 +16,33 @@
 
 #define PRU_RPROC_DRVNAME "pru-rproc"
 
+/*
+ * 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,
+	PRUSS_GPI_MODE_MAX,
+};
+
+/**
+ * enum pru_type - PRU core type identifier
+ *
+ * @PRU_TYPE_PRU: Programmable Real-time Unit
+ * @PRU_TYPE_RTU: Auxiliary Programmable Real-Time Unit
+ * @PRU_TYPE_TX_PRU: Transmit Programmable Real-Time Unit
+ * @PRU_TYPE_MAX: just keep this one at the end
+ */
+enum pru_type {
+	PRU_TYPE_PRU = 0,
+	PRU_TYPE_RTU,
+	PRU_TYPE_TX_PRU,
+	PRU_TYPE_MAX,
+};
+
 /**
  * enum pruss_pru_id - PRU core identifiers
  * @PRUSS_PRU0: PRU Core 0.
@@ -78,6 +105,11 @@ 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, enum pru_type pru_type,
+			 bool enable);
 
 #else
 
@@ -101,6 +133,25 @@ 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,
+				       enum pru_type pru_type,
+				       bool enable);
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif /* CONFIG_TI_PRUSS */
 
 #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
-- 
2.25.1

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

[MD Danish Anwar]

From: Tero Kristo <t-kristo@ti.com>

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 <s-anna@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/soc/ti/pruss.c           | 44 ++++++++++++++++++++++++++++++++
 include/linux/remoteproc/pruss.h | 30 ++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index ac415442e85b..3aa3c38c6c79 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -239,6 +239,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
 }
 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 bb001f712980..42f1586c62ac 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -16,6 +16,24 @@
 
 #define PRU_RPROC_DRVNAME "pru-rproc"
 
+/*
+ * 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
@@ -110,6 +128,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
 int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
 int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
 			 bool enable);
+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
 
@@ -152,6 +172,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss,
 	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

Re: [PATCH v5 0/5] Introduce PRU platform consumer API

[Tony Lindgren]

* MD Danish Anwar <danishanwar@ti.com> [230323 06:25]:
> The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS
> or simply PRUSS) on various TI SoCs consists of dual 32-bit RISC cores
> (Programmable Real-Time Units, or PRUs) for program execution.

Thanks this series looks good to me now:

Reviewed-by: Tony Lindgren <tony@atomide.com>

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

[Roger Quadros]

On 23/03/2023 08:24, MD Danish Anwar wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> 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 <s-anna@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

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

[Roger Quadros]

On 23/03/2023 08:24, MD Danish Anwar wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> 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 <s-anna@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH v5 0/5] Introduce PRU platform consumer API

[Md Danish Anwar]

Hi Mathieu,

On 23/03/23 11:54, MD Danish Anwar wrote:
> Hi All,
> The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS
> or simply PRUSS) on various TI SoCs consists of dual 32-bit RISC cores
> (Programmable Real-Time Units, or PRUs) for program execution.
> 
> There are 3 foundation components for TI PRUSS subsystem: the PRUSS platform
> driver, the PRUSS INTC driver and the PRUSS remoteproc driver. All of them have
> already been merged and can be found under:
> 1) drivers/soc/ti/pruss.c
>    Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
> 2) drivers/irqchip/irq-pruss-intc.c
>    Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml
> 3) drivers/remoteproc/pru_rproc.c
>    Documentation/devicetree/bindings/remoteproc/ti,pru-consumer.yaml
> 
> The programmable nature of the PRUs provide flexibility to implement custom
> peripheral interfaces, fast real-time responses, or specialized data handling.
> Example of a PRU consumer drivers will be: 
>   - Software UART over PRUSS
>   - PRU-ICSS Ethernet EMAC
> 
> In order to make usage of common PRU resources and allow the consumer drivers 
> to configure the PRU hardware for specific usage the PRU API is introduced.
>
Roger has given his RBs for all the patches in this series. Tony has also given
his RB.

Can you please have a look at this series.

-- 
Thanks and Regards,
Danish.

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

[Mathieu Poirier]

Hi Danish

On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
> From: Tero Kristo <t-kristo@ti.com>
> 
> 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 <s-anna@ti.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/remoteproc/pru_rproc.c                |  2 +-
>  drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
>  .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
>  include/linux/remoteproc/pruss.h              | 19 ++++++
>  4 files changed, 83 insertions(+), 5 deletions(-)
>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
> 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index b76db7fa693d..4ddd5854d56e 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -19,7 +19,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/remoteproc/pruss.h>
> -#include <linux/pruss_driver.h>
> +#include <linux/pruss_internal.h>
>  #include <linux/remoteproc.h>
>  
>  #include "remoteproc_internal.h"
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 6882c86b3ce5..6c2bb02a521d 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -6,6 +6,7 @@
>   * Author(s):
>   *	Suman Anna <s-anna@ti.com>
>   *	Andrew F. Davis <afd@ti.com>
> + *	Tero Kristo <t-kristo@ti.com>
>   */
>  
>  #include <linux/clk-provider.h>
> @@ -16,8 +17,9 @@
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/pruss_driver.h>
> +#include <linux/pruss_internal.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);
> +

There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
or pru_rproc_get().

> +	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_internal.h
> similarity index 90%
> rename from include/linux/pruss_driver.h
> rename to include/linux/pruss_internal.h
> index ecfded30ed05..8f91cb164054 100644
> --- a/include/linux/pruss_driver.h
> +++ b/include/linux/pruss_internal.h
> @@ -6,9 +6,10 @@
>   *	Suman Anna <s-anna@ti.com>
>   */
>  
> -#ifndef _PRUSS_DRIVER_H_
> -#define _PRUSS_DRIVER_H_
> +#ifndef _PRUSS_INTERNAL_H_
> +#define _PRUSS_INTERNAL_H_
>  
> +#include <linux/remoteproc/pruss.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -51,4 +52,4 @@ struct pruss {
>  	struct clk *iep_clk_mux;
>  };
>  
> -#endif	/* _PRUSS_DRIVER_H_ */
> +#endif	/* _PRUSS_INTERNAL_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 <s-anna@ti.com>
> + *	Tero Kristo <t-kristo@ti.com>
>   */
>  
>  #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
> 

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

[Mathieu Poirier]

On Thu, Mar 23, 2023 at 11:54:48AM +0530, MD Danish Anwar wrote:
> From: "Andrew F. Davis" <afd@ti.com>
> 
> Add two new API - pruss_request_mem_region() & pruss_release_mem_region(),
> to the PRUSS platform driver to allow client drivers to acquire and release
> the common memory resources present within a PRU-ICSS subsystem. This
> allows the client drivers to directly manipulate the respective memories,
> as per their design contract with the associated firmware.
> 
> Co-developed-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> ---
>  drivers/soc/ti/pruss.c           | 77 ++++++++++++++++++++++++++++++++
>  include/linux/pruss_internal.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 6c2bb02a521d..126b672b9b30 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_internal.h b/include/linux/pruss_internal.h
> index 8f91cb164054..cf5287fa01df 100644
> --- a/include/linux/pruss_internal.h
> +++ b/include/linux/pruss_internal.h
> @@ -9,37 +9,18 @@
>  #ifndef _PRUSS_INTERNAL_H_
>  #define _PRUSS_INTERNAL_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
> 

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

[Mathieu Poirier]

On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> 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 <s-anna@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/soc/ti/pruss.c |   1 +
>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 113 insertions(+)
>  create mode 100644 drivers/soc/ti/pruss.h
>

This patch doesn't compile without warnings.

> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 126b672b9b30..2fa7df667592 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -21,6 +21,7 @@
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
>  #include <linux/slab.h>
> +#include "pruss.h"
>  
>  /**
>   * struct pruss_private_data - PRUSS driver private data
> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
> new file mode 100644
> index 000000000000..4626d5f6b874
> --- /dev/null
> +++ b/drivers/soc/ti/pruss.h
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * PRU-ICSS Subsystem user interfaces
> + *
> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
> + *	MD Danish Anwar <danishanwar@ti.com>
> + */
> +
> +#ifndef _SOC_TI_PRUSS_H_
> +#define _SOC_TI_PRUSS_H_
> +
> +#include <linux/bits.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * 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)
> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
> +
> +/**
> + * 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);
> +}
> +
> +#endif  /* _SOC_TI_PRUSS_H_ */
> -- 
> 2.25.1
> 

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

[Mathieu Poirier]

On Thu, Mar 23, 2023 at 11:54:51AM +0530, MD Danish Anwar wrote:
> From: Tero Kristo <t-kristo@ti.com>
> 
> 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 <s-anna@ti.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/soc/ti/pruss.c           | 44 ++++++++++++++++++++++++++++++++
>  include/linux/remoteproc/pruss.h | 30 ++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index ac415442e85b..3aa3c38c6c79 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -239,6 +239,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
>  }
>  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);

What happens if @mux is NULL?

Thanks,
Mathieu


> +	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 bb001f712980..42f1586c62ac 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -16,6 +16,24 @@
>  
>  #define PRU_RPROC_DRVNAME "pru-rproc"
>  
> +/*
> + * 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
> @@ -110,6 +128,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>  int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
>  int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
>  			 bool enable);
> +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
>  
> @@ -152,6 +172,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss,
>  	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
> 

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

[Md Danish Anwar]

Hi Mathieu,

On 28/03/23 02:28, Mathieu Poirier wrote:
> Hi Danish
> 
> On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
>> From: Tero Kristo <t-kristo@ti.com>
>>
>> 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 <s-anna@ti.com>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/remoteproc/pru_rproc.c                |  2 +-
>>  drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
>>  .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
>>  include/linux/remoteproc/pruss.h              | 19 ++++++
>>  4 files changed, 83 insertions(+), 5 deletions(-)
>>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>>
>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>> index b76db7fa693d..4ddd5854d56e 100644
>> --- a/drivers/remoteproc/pru_rproc.c
>> +++ b/drivers/remoteproc/pru_rproc.c
>> @@ -19,7 +19,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/remoteproc/pruss.h>
>> -#include <linux/pruss_driver.h>
>> +#include <linux/pruss_internal.h>
>>  #include <linux/remoteproc.h>
>>  
>>  #include "remoteproc_internal.h"
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 6882c86b3ce5..6c2bb02a521d 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -6,6 +6,7 @@
>>   * Author(s):
>>   *	Suman Anna <s-anna@ti.com>
>>   *	Andrew F. Davis <afd@ti.com>
>> + *	Tero Kristo <t-kristo@ti.com>
>>   */
>>  
>>  #include <linux/clk-provider.h>
>> @@ -16,8 +17,9 @@
>>  #include <linux/of_address.h>
>>  #include <linux/of_device.h>
>>  #include <linux/pm_runtime.h>
>> -#include <linux/pruss_driver.h>
>> +#include <linux/pruss_internal.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);
>> +
> 
> There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
> or pru_rproc_get().
> 

Here in this API, we are checking if rproc is NULL or not. Also we are checking
is_pru_rproc() to make sure this rproc is pru-rproc only and not some other rproc.

This API will be called from driver (icssg_prueth.c) which I'll post once this
series is merged.

In the driver we are doing,

	prueth->pru[slice] = pru_rproc_get(np, pru, &pruss_id);

	pruss = pruss_get(prueth->pru[slice]);

So, before calling pruss_get() we are in fact calling pru_rproc_get() to make
sure it's a valid rproc.

I think in this API, these two checks (NULL check and is_pru_rproc) should be
OK as the driver is already calling pru_rproc_get() before this API.

The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
is_pru_rproc() will only be true if it is a "pru-rproc" implying
pru_rproc_get() was called before calling this API.

Please let me know if this is OK or if any change is required.

>> +	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_internal.h
>> similarity index 90%
>> rename from include/linux/pruss_driver.h
>> rename to include/linux/pruss_internal.h
>> index ecfded30ed05..8f91cb164054 100644
>> --- a/include/linux/pruss_driver.h
>> +++ b/include/linux/pruss_internal.h
>> @@ -6,9 +6,10 @@
>>   *	Suman Anna <s-anna@ti.com>
>>   */
>>  
>> -#ifndef _PRUSS_DRIVER_H_
>> -#define _PRUSS_DRIVER_H_
>> +#ifndef _PRUSS_INTERNAL_H_
>> +#define _PRUSS_INTERNAL_H_
>>  
>> +#include <linux/remoteproc/pruss.h>
>>  #include <linux/types.h>
>>  
>>  /*
>> @@ -51,4 +52,4 @@ struct pruss {
>>  	struct clk *iep_clk_mux;
>>  };
>>  
>> -#endif	/* _PRUSS_DRIVER_H_ */
>> +#endif	/* _PRUSS_INTERNAL_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 <s-anna@ti.com>
>> + *	Tero Kristo <t-kristo@ti.com>
>>   */
>>  
>>  #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
>>

-- 
Thanks and Regards,
Danish.

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

[Md Danish Anwar]

On 28/03/23 02:31, Mathieu Poirier wrote:
> On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
>> From: Suman Anna <s-anna@ti.com>
>>
>> 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 <s-anna@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/soc/ti/pruss.c |   1 +
>>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 113 insertions(+)
>>  create mode 100644 drivers/soc/ti/pruss.h
>>
> 
> This patch doesn't compile without warnings.
> 

Sure, Mathieu. I'll check the warnings.

>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 126b672b9b30..2fa7df667592 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/regmap.h>
>>  #include <linux/remoteproc.h>
>>  #include <linux/slab.h>
>> +#include "pruss.h"
>>  
>>  /**
>>   * struct pruss_private_data - PRUSS driver private data
>> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
>> new file mode 100644
>> index 000000000000..4626d5f6b874
>> --- /dev/null
>> +++ b/drivers/soc/ti/pruss.h
>> @@ -0,0 +1,112 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * PRU-ICSS Subsystem user interfaces
>> + *
>> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
>> + *	MD Danish Anwar <danishanwar@ti.com>
>> + */
>> +
>> +#ifndef _SOC_TI_PRUSS_H_
>> +#define _SOC_TI_PRUSS_H_
>> +
>> +#include <linux/bits.h>
>> +#include <linux/regmap.h>
>> +
>> +/*
>> + * 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)
>> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
>> +
>> +/**
>> + * 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);
>> +}
>> +
>> +#endif  /* _SOC_TI_PRUSS_H_ */
>> -- 
>> 2.25.1
>>

-- 
Thanks and Regards,
Danish.

Re: [EXTERNAL] Re: [PATCH v5 5/5] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

[Md Danish Anwar]

On 28/03/23 02:34, Mathieu Poirier wrote:
> On Thu, Mar 23, 2023 at 11:54:51AM +0530, MD Danish Anwar wrote:
>> From: Tero Kristo <t-kristo@ti.com>
>>
>> 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 <s-anna@ti.com>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/soc/ti/pruss.c           | 44 ++++++++++++++++++++++++++++++++
>>  include/linux/remoteproc/pruss.h | 30 ++++++++++++++++++++++
>>  2 files changed, 74 insertions(+)
>>
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index ac415442e85b..3aa3c38c6c79 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -239,6 +239,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
>>  }
>>  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);
> 
> What happens if @mux is NULL?

@mux being null may result in some error here. I will add NULL check for mux
before storing the value in mux.

I will modify the above if condition to have NULL check for mux as well.
The if condition will look like below.

	if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS || !mux)
		return -EINVAL;

Please let me know if this looks OK.

> 
> Thanks,
> Mathieu
> 
> 
>> +	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 bb001f712980..42f1586c62ac 100644
>> --- a/include/linux/remoteproc/pruss.h
>> +++ b/include/linux/remoteproc/pruss.h
>> @@ -16,6 +16,24 @@
>>  
>>  #define PRU_RPROC_DRVNAME "pru-rproc"
>>  
>> +/*
>> + * 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
>> @@ -110,6 +128,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>  int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
>>  int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
>>  			 bool enable);
>> +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
>>  
>> @@ -152,6 +172,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss,
>>  	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
>>

-- 
Thanks and Regards,
Danish.

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

[Mathieu Poirier]

On Mon, 27 Mar 2023 at 23:42, Md Danish Anwar <a0501179@ti.com> wrote:
>
> Hi Mathieu,
>
> On 28/03/23 02:28, Mathieu Poirier wrote:
> > Hi Danish
> >
> > On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
> >> From: Tero Kristo <t-kristo@ti.com>
> >>
> >> 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 <s-anna@ti.com>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  drivers/remoteproc/pru_rproc.c                |  2 +-
> >>  drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
> >>  .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
> >>  include/linux/remoteproc/pruss.h              | 19 ++++++
> >>  4 files changed, 83 insertions(+), 5 deletions(-)
> >>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
> >>
> >> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> >> index b76db7fa693d..4ddd5854d56e 100644
> >> --- a/drivers/remoteproc/pru_rproc.c
> >> +++ b/drivers/remoteproc/pru_rproc.c
> >> @@ -19,7 +19,7 @@
> >>  #include <linux/of_device.h>
> >>  #include <linux/of_irq.h>
> >>  #include <linux/remoteproc/pruss.h>
> >> -#include <linux/pruss_driver.h>
> >> +#include <linux/pruss_internal.h>
> >>  #include <linux/remoteproc.h>
> >>
> >>  #include "remoteproc_internal.h"
> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> >> index 6882c86b3ce5..6c2bb02a521d 100644
> >> --- a/drivers/soc/ti/pruss.c
> >> +++ b/drivers/soc/ti/pruss.c
> >> @@ -6,6 +6,7 @@
> >>   * Author(s):
> >>   *  Suman Anna <s-anna@ti.com>
> >>   *  Andrew F. Davis <afd@ti.com>
> >> + *  Tero Kristo <t-kristo@ti.com>
> >>   */
> >>
> >>  #include <linux/clk-provider.h>
> >> @@ -16,8 +17,9 @@
> >>  #include <linux/of_address.h>
> >>  #include <linux/of_device.h>
> >>  #include <linux/pm_runtime.h>
> >> -#include <linux/pruss_driver.h>
> >> +#include <linux/pruss_internal.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);
> >> +
> >
> > There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
> > or pru_rproc_get().
> >
>
> Here in this API, we are checking if rproc is NULL or not. Also we are checking
> is_pru_rproc() to make sure this rproc is pru-rproc only and not some other rproc.
>
> This API will be called from driver (icssg_prueth.c) which I'll post once this
> series is merged.
>
> In the driver we are doing,
>
>         prueth->pru[slice] = pru_rproc_get(np, pru, &pruss_id);
>
>         pruss = pruss_get(prueth->pru[slice]);
>
> So, before calling pruss_get() we are in fact calling pru_rproc_get() to make
> sure it's a valid rproc.
>

You are doing the right thing but because pruss_get() is exported
globally, other people eventually using the interface may not be so
rigorous.  Add a comment to the pruss_get() function documentation
clearly mentioning it is expected the caller will have done a
pru_rproc_get() on @rproc.

> I think in this API, these two checks (NULL check and is_pru_rproc) should be
> OK as the driver is already calling pru_rproc_get() before this API.
>
> The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
> is_pru_rproc() will only be true if it is a "pru-rproc" implying
> pru_rproc_get() was called before calling this API.
>
> Please let me know if this is OK or if any change is required.
>
> >> +    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_internal.h
> >> similarity index 90%
> >> rename from include/linux/pruss_driver.h
> >> rename to include/linux/pruss_internal.h
> >> index ecfded30ed05..8f91cb164054 100644
> >> --- a/include/linux/pruss_driver.h
> >> +++ b/include/linux/pruss_internal.h
> >> @@ -6,9 +6,10 @@
> >>   *  Suman Anna <s-anna@ti.com>
> >>   */
> >>
> >> -#ifndef _PRUSS_DRIVER_H_
> >> -#define _PRUSS_DRIVER_H_
> >> +#ifndef _PRUSS_INTERNAL_H_
> >> +#define _PRUSS_INTERNAL_H_
> >>
> >> +#include <linux/remoteproc/pruss.h>
> >>  #include <linux/types.h>
> >>
> >>  /*
> >> @@ -51,4 +52,4 @@ struct pruss {
> >>      struct clk *iep_clk_mux;
> >>  };
> >>
> >> -#endif      /* _PRUSS_DRIVER_H_ */
> >> +#endif      /* _PRUSS_INTERNAL_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 <s-anna@ti.com>
> >> + *  Tero Kristo <t-kristo@ti.com>
> >>   */
> >>
> >>  #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
> >>
>
> --
> Thanks and Regards,
> Danish.

Re: [EXTERNAL] Re: [PATCH v5 5/5] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

[Mathieu Poirier]

On Tue, 28 Mar 2023 at 05:28, Md Danish Anwar <a0501179@ti.com> wrote:
>
>
>
> On 28/03/23 02:34, Mathieu Poirier wrote:
> > On Thu, Mar 23, 2023 at 11:54:51AM +0530, MD Danish Anwar wrote:
> >> From: Tero Kristo <t-kristo@ti.com>
> >>
> >> 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 <s-anna@ti.com>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  drivers/soc/ti/pruss.c           | 44 ++++++++++++++++++++++++++++++++
> >>  include/linux/remoteproc/pruss.h | 30 ++++++++++++++++++++++
> >>  2 files changed, 74 insertions(+)
> >>
> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> >> index ac415442e85b..3aa3c38c6c79 100644
> >> --- a/drivers/soc/ti/pruss.c
> >> +++ b/drivers/soc/ti/pruss.c
> >> @@ -239,6 +239,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
> >>  }
> >>  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);
> >
> > What happens if @mux is NULL?
>
> @mux being null may result in some error here. I will add NULL check for mux
> before storing the value in mux.
>

It will result in a kernel panic.

> I will modify the above if condition to have NULL check for mux as well.
> The if condition will look like below.
>
>         if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS || !mux)
>                 return -EINVAL;
>

That will be fine.

> Please let me know if this looks OK.
>
> >
> > Thanks,
> > Mathieu
> >
> >
> >> +    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 bb001f712980..42f1586c62ac 100644
> >> --- a/include/linux/remoteproc/pruss.h
> >> +++ b/include/linux/remoteproc/pruss.h
> >> @@ -16,6 +16,24 @@
> >>
> >>  #define PRU_RPROC_DRVNAME "pru-rproc"
> >>
> >> +/*
> >> + * 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
> >> @@ -110,6 +128,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
> >>  int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
> >>  int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
> >>                       bool enable);
> >> +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
> >>
> >> @@ -152,6 +172,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss,
> >>      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
> >>
>
> --
> Thanks and Regards,
> Danish.

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

[Md Danish Anwar]

On 29/03/23 23:29, Mathieu Poirier wrote:
> On Mon, 27 Mar 2023 at 23:42, Md Danish Anwar <a0501179@ti.com> wrote:
>>
>> Hi Mathieu,
>>
>> On 28/03/23 02:28, Mathieu Poirier wrote:
>>> Hi Danish
>>>
>>> On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
>>>> From: Tero Kristo <t-kristo@ti.com>
>>>>
>>>> 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 <s-anna@ti.com>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>  drivers/remoteproc/pru_rproc.c                |  2 +-
>>>>  drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
>>>>  .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
>>>>  include/linux/remoteproc/pruss.h              | 19 ++++++
>>>>  4 files changed, 83 insertions(+), 5 deletions(-)
>>>>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>>>>
>>>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>>>> index b76db7fa693d..4ddd5854d56e 100644
>>>> --- a/drivers/remoteproc/pru_rproc.c
>>>> +++ b/drivers/remoteproc/pru_rproc.c
>>>> @@ -19,7 +19,7 @@
>>>>  #include <linux/of_device.h>
>>>>  #include <linux/of_irq.h>
>>>>  #include <linux/remoteproc/pruss.h>
>>>> -#include <linux/pruss_driver.h>
>>>> +#include <linux/pruss_internal.h>
>>>>  #include <linux/remoteproc.h>
>>>>
>>>>  #include "remoteproc_internal.h"
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index 6882c86b3ce5..6c2bb02a521d 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -6,6 +6,7 @@
>>>>   * Author(s):
>>>>   *  Suman Anna <s-anna@ti.com>
>>>>   *  Andrew F. Davis <afd@ti.com>
>>>> + *  Tero Kristo <t-kristo@ti.com>
>>>>   */
>>>>
>>>>  #include <linux/clk-provider.h>
>>>> @@ -16,8 +17,9 @@
>>>>  #include <linux/of_address.h>
>>>>  #include <linux/of_device.h>
>>>>  #include <linux/pm_runtime.h>
>>>> -#include <linux/pruss_driver.h>
>>>> +#include <linux/pruss_internal.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);
>>>> +
>>>
>>> There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
>>> or pru_rproc_get().
>>>
>>
>> Here in this API, we are checking if rproc is NULL or not. Also we are checking
>> is_pru_rproc() to make sure this rproc is pru-rproc only and not some other rproc.
>>
>> This API will be called from driver (icssg_prueth.c) which I'll post once this
>> series is merged.
>>
>> In the driver we are doing,
>>
>>         prueth->pru[slice] = pru_rproc_get(np, pru, &pruss_id);
>>
>>         pruss = pruss_get(prueth->pru[slice]);
>>
>> So, before calling pruss_get() we are in fact calling pru_rproc_get() to make
>> sure it's a valid rproc.
>>
> 
> You are doing the right thing but because pruss_get() is exported
> globally, other people eventually using the interface may not be so
> rigorous.  Add a comment to the pruss_get() function documentation
> clearly mentioning it is expected the caller will have done a
> pru_rproc_get() on @rproc.
> 

Sure, Mathieu. I will add the required comment in pruss_get() documentation.

>> I think in this API, these two checks (NULL check and is_pru_rproc) should be
>> OK as the driver is already calling pru_rproc_get() before this API.
>>
>> The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
>> is_pru_rproc() will only be true if it is a "pru-rproc" implying
>> pru_rproc_get() was called before calling this API.
>>
>> Please let me know if this is OK or if any change is required.
>>
>>>> +    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_internal.h
>>>> similarity index 90%
>>>> rename from include/linux/pruss_driver.h
>>>> rename to include/linux/pruss_internal.h
>>>> index ecfded30ed05..8f91cb164054 100644
>>>> --- a/include/linux/pruss_driver.h
>>>> +++ b/include/linux/pruss_internal.h
>>>> @@ -6,9 +6,10 @@
>>>>   *  Suman Anna <s-anna@ti.com>
>>>>   */
>>>>
>>>> -#ifndef _PRUSS_DRIVER_H_
>>>> -#define _PRUSS_DRIVER_H_
>>>> +#ifndef _PRUSS_INTERNAL_H_
>>>> +#define _PRUSS_INTERNAL_H_
>>>>
>>>> +#include <linux/remoteproc/pruss.h>
>>>>  #include <linux/types.h>
>>>>
>>>>  /*
>>>> @@ -51,4 +52,4 @@ struct pruss {
>>>>      struct clk *iep_clk_mux;
>>>>  };
>>>>
>>>> -#endif      /* _PRUSS_DRIVER_H_ */
>>>> +#endif      /* _PRUSS_INTERNAL_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 <s-anna@ti.com>
>>>> + *  Tero Kristo <t-kristo@ti.com>
>>>>   */
>>>>
>>>>  #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
>>>>
>>
>> --
>> Thanks and Regards,
>> Danish.

-- 
Thanks and Regards,
Danish.

Re: [EXTERNAL] Re: [PATCH v5 5/5] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

[Md Danish Anwar]

On 29/03/23 23:34, Mathieu Poirier wrote:
> On Tue, 28 Mar 2023 at 05:28, Md Danish Anwar <a0501179@ti.com> wrote:
>>
>> On 28/03/23 02:34, Mathieu Poirier wrote:
>>> On Thu, Mar 23, 2023 at 11:54:51AM +0530, MD Danish Anwar wrote:
>>>> From: Tero Kristo <t-kristo@ti.com>
>>>>
>>>> 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 <s-anna@ti.com>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>  drivers/soc/ti/pruss.c           | 44 ++++++++++++++++++++++++++++++++
>>>>  include/linux/remoteproc/pruss.h | 30 ++++++++++++++++++++++
>>>>  2 files changed, 74 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index ac415442e85b..3aa3c38c6c79 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -239,6 +239,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
>>>>  }
>>>>  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);
>>>
>>> What happens if @mux is NULL?
>>
>> @mux being null may result in some error here. I will add NULL check for mux
>> before storing the value in mux.
>>
> 
> It will result in a kernel panic.
> 
>> I will modify the above if condition to have NULL check for mux as well.
>> The if condition will look like below.
>>
>>         if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS || !mux)
>>                 return -EINVAL;
>>
> 
> That will be fine.>

Sure, I'll go ahead and make this change.

>> Please let me know if this looks OK.
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>
>>>> +    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 bb001f712980..42f1586c62ac 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -16,6 +16,24 @@
>>>>
>>>>  #define PRU_RPROC_DRVNAME "pru-rproc"
>>>>
>>>> +/*
>>>> + * 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
>>>> @@ -110,6 +128,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
>>>>  int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
>>>>  int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
>>>>                       bool enable);
>>>> +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
>>>>
>>>> @@ -152,6 +172,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss,
>>>>      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
>>>>
>>
>> --
>> Thanks and Regards,
>> Danish.

-- 
Thanks and Regards,
Danish.

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

[Md Danish Anwar]

Hi Mathieu,

On 28/03/23 02:31, Mathieu Poirier wrote:
> On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
>> From: Suman Anna <s-anna@ti.com>
>>
>> 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 <s-anna@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/soc/ti/pruss.c |   1 +
>>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 113 insertions(+)
>>  create mode 100644 drivers/soc/ti/pruss.h
>>
> 
> This patch doesn't compile without warnings.
> 

I checked the warnings. Below are the warnings that I am getting for these patch.

In file included from drivers/soc/ti/pruss.c:24:
drivers/soc/ti/pruss.h:103:12: warning: ‘pruss_cfg_update’ defined but not used
[-Wunused-function]
  103 | static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
      |            ^~~~~~~~~~~~~~~~
drivers/soc/ti/pruss.h:84:12: warning: ‘pruss_cfg_read’ defined but not used
[-Wunused-function]
   84 | static int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
unsigned int *val)

These warnings are coming because pruss_cfg_read() / update() APIs are
introduced in this patch but they are used later.

One way to resolve this warning is to make this API "inline". I compiled after
making these APIs inline, it got compiled without any warnings.

The other solution is to merge a user API of these APIs in this patch. Patch 4
and 5 introduces some APIs that uses pruss_cfg_read() / update() APIs. If we
squash patch 5 (as patch 5 uses both read() and update() APIs where as patch 4
only uses update() API) with this patch and make it a single patch where
pruss_cfg_read() / update() is introduced as well as used, then this warning
will be resolved.

I still think making these APIs "inline" is a better option as these APIs
implement very simple one line logic and can be made inline.

Please let me know what do you think and which approach sounds better.


>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 126b672b9b30..2fa7df667592 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/regmap.h>
>>  #include <linux/remoteproc.h>
>>  #include <linux/slab.h>
>> +#include "pruss.h"
>>  
>>  /**
>>   * struct pruss_private_data - PRUSS driver private data
>> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
>> new file mode 100644
>> index 000000000000..4626d5f6b874
>> --- /dev/null
>> +++ b/drivers/soc/ti/pruss.h
>> @@ -0,0 +1,112 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * PRU-ICSS Subsystem user interfaces
>> + *
>> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
>> + *	MD Danish Anwar <danishanwar@ti.com>
>> + */
>> +
>> +#ifndef _SOC_TI_PRUSS_H_
>> +#define _SOC_TI_PRUSS_H_
>> +
>> +#include <linux/bits.h>
>> +#include <linux/regmap.h>
>> +
>> +/*
>> + * 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)
>> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
>> +
>> +/**
>> + * 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);
>> +}
>> +
>> +#endif  /* _SOC_TI_PRUSS_H_ */
>> -- 
>> 2.25.1
>>

-- 
Thanks and Regards,
Danish.

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

[Mathieu Poirier]

On Thu, 30 Mar 2023 at 04:00, Md Danish Anwar <a0501179@ti.com> wrote:
>
> Hi Mathieu,
>
> On 28/03/23 02:31, Mathieu Poirier wrote:
> > On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
> >> From: Suman Anna <s-anna@ti.com>
> >>
> >> 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 <s-anna@ti.com>
> >> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >> ---
> >>  drivers/soc/ti/pruss.c |   1 +
> >>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 113 insertions(+)
> >>  create mode 100644 drivers/soc/ti/pruss.h
> >>
> >
> > This patch doesn't compile without warnings.
> >
>
> I checked the warnings. Below are the warnings that I am getting for these patch.
>
> In file included from drivers/soc/ti/pruss.c:24:
> drivers/soc/ti/pruss.h:103:12: warning: ‘pruss_cfg_update’ defined but not used
> [-Wunused-function]
>   103 | static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>       |            ^~~~~~~~~~~~~~~~
> drivers/soc/ti/pruss.h:84:12: warning: ‘pruss_cfg_read’ defined but not used
> [-Wunused-function]
>    84 | static int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
> unsigned int *val)
>
> These warnings are coming because pruss_cfg_read() / update() APIs are
> introduced in this patch but they are used later.
>
> One way to resolve this warning is to make this API "inline". I compiled after
> making these APIs inline, it got compiled without any warnings.
>
> The other solution is to merge a user API of these APIs in this patch. Patch 4
> and 5 introduces some APIs that uses pruss_cfg_read() / update() APIs. If we
> squash patch 5 (as patch 5 uses both read() and update() APIs where as patch 4
> only uses update() API) with this patch and make it a single patch where
> pruss_cfg_read() / update() is introduced as well as used, then this warning
> will be resolved.
>

The proper way to do this is to introduce new APIs only when they are needed.

> I still think making these APIs "inline" is a better option as these APIs
> implement very simple one line logic and can be made inline.
>
> Please let me know what do you think and which approach sounds better.
>
>
> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> >> index 126b672b9b30..2fa7df667592 100644
> >> --- a/drivers/soc/ti/pruss.c
> >> +++ b/drivers/soc/ti/pruss.c
> >> @@ -21,6 +21,7 @@
> >>  #include <linux/regmap.h>
> >>  #include <linux/remoteproc.h>
> >>  #include <linux/slab.h>
> >> +#include "pruss.h"
> >>
> >>  /**
> >>   * struct pruss_private_data - PRUSS driver private data
> >> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
> >> new file mode 100644
> >> index 000000000000..4626d5f6b874
> >> --- /dev/null
> >> +++ b/drivers/soc/ti/pruss.h
> >> @@ -0,0 +1,112 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * PRU-ICSS Subsystem user interfaces
> >> + *
> >> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
> >> + *  MD Danish Anwar <danishanwar@ti.com>
> >> + */
> >> +
> >> +#ifndef _SOC_TI_PRUSS_H_
> >> +#define _SOC_TI_PRUSS_H_
> >> +
> >> +#include <linux/bits.h>
> >> +#include <linux/regmap.h>
> >> +
> >> +/*
> >> + * 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)
> >> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
> >> +
> >> +/**
> >> + * 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);
> >> +}
> >> +
> >> +#endif  /* _SOC_TI_PRUSS_H_ */
> >> --
> >> 2.25.1
> >>
>
> --
> Thanks and Regards,
> Danish.

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

[Roger Quadros]

On 30/03/2023 13:00, Md Danish Anwar wrote:
> Hi Mathieu,
> 
> On 28/03/23 02:31, Mathieu Poirier wrote:
>> On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
>>> From: Suman Anna <s-anna@ti.com>
>>>
>>> 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 <s-anna@ti.com>
>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---
>>>  drivers/soc/ti/pruss.c |   1 +
>>>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 113 insertions(+)
>>>  create mode 100644 drivers/soc/ti/pruss.h
>>>
>>
>> This patch doesn't compile without warnings.
>>
> 
> I checked the warnings. Below are the warnings that I am getting for these patch.
> 
> In file included from drivers/soc/ti/pruss.c:24:
> drivers/soc/ti/pruss.h:103:12: warning: ‘pruss_cfg_update’ defined but not used
> [-Wunused-function]
>   103 | static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>       |            ^~~~~~~~~~~~~~~~
> drivers/soc/ti/pruss.h:84:12: warning: ‘pruss_cfg_read’ defined but not used
> [-Wunused-function]
>    84 | static int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
> unsigned int *val)
> 
> These warnings are coming because pruss_cfg_read() / update() APIs are
> introduced in this patch but they are used later.
> 
> One way to resolve this warning is to make this API "inline". I compiled after
> making these APIs inline, it got compiled without any warnings.
> 
> The other solution is to merge a user API of these APIs in this patch. Patch 4
> and 5 introduces some APIs that uses pruss_cfg_read() / update() APIs. If we
> squash patch 5 (as patch 5 uses both read() and update() APIs where as patch 4
> only uses update() API) with this patch and make it a single patch where
> pruss_cfg_read() / update() is introduced as well as used, then this warning
> will be resolved.
> 
> I still think making these APIs "inline" is a better option as these APIs
> implement very simple one line logic and can be made inline.
> 
> Please let me know what do you think and which approach sounds better.

You should squash this patch with the next one.

> 
> 
>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>> index 126b672b9b30..2fa7df667592 100644
>>> --- a/drivers/soc/ti/pruss.c
>>> +++ b/drivers/soc/ti/pruss.c
>>> @@ -21,6 +21,7 @@
>>>  #include <linux/regmap.h>
>>>  #include <linux/remoteproc.h>
>>>  #include <linux/slab.h>
>>> +#include "pruss.h"
>>>  
>>>  /**
>>>   * struct pruss_private_data - PRUSS driver private data
>>> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
>>> new file mode 100644
>>> index 000000000000..4626d5f6b874
>>> --- /dev/null
>>> +++ b/drivers/soc/ti/pruss.h
>>> @@ -0,0 +1,112 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * PRU-ICSS Subsystem user interfaces
>>> + *
>>> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
>>> + *	MD Danish Anwar <danishanwar@ti.com>
>>> + */
>>> +
>>> +#ifndef _SOC_TI_PRUSS_H_
>>> +#define _SOC_TI_PRUSS_H_
>>> +
>>> +#include <linux/bits.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +/*
>>> + * 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)
>>> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
>>> +
>>> +/**
>>> + * 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);
>>> +}
>>> +
>>> +#endif  /* _SOC_TI_PRUSS_H_ */
>>> -- 
>>> 2.25.1
>>>
> 

--
cheers,
-roger

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

[Md Danish Anwar]

On 30/03/23 19:51, Mathieu Poirier wrote:
> On Thu, 30 Mar 2023 at 04:00, Md Danish Anwar <a0501179@ti.com> wrote:
>>
>> Hi Mathieu,
>>
>> On 28/03/23 02:31, Mathieu Poirier wrote:
>>> On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
>>>> From: Suman Anna <s-anna@ti.com>
>>>>
>>>> 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 <s-anna@ti.com>
>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> ---
>>>>  drivers/soc/ti/pruss.c |   1 +
>>>>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 113 insertions(+)
>>>>  create mode 100644 drivers/soc/ti/pruss.h
>>>>
>>>
>>> This patch doesn't compile without warnings.
>>>
>>
>> I checked the warnings. Below are the warnings that I am getting for these patch.
>>
>> In file included from drivers/soc/ti/pruss.c:24:
>> drivers/soc/ti/pruss.h:103:12: warning: ‘pruss_cfg_update’ defined but not used
>> [-Wunused-function]
>>   103 | static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>       |            ^~~~~~~~~~~~~~~~
>> drivers/soc/ti/pruss.h:84:12: warning: ‘pruss_cfg_read’ defined but not used
>> [-Wunused-function]
>>    84 | static int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
>> unsigned int *val)
>>
>> These warnings are coming because pruss_cfg_read() / update() APIs are
>> introduced in this patch but they are used later.
>>
>> One way to resolve this warning is to make this API "inline". I compiled after
>> making these APIs inline, it got compiled without any warnings.
>>
>> The other solution is to merge a user API of these APIs in this patch. Patch 4
>> and 5 introduces some APIs that uses pruss_cfg_read() / update() APIs. If we
>> squash patch 5 (as patch 5 uses both read() and update() APIs where as patch 4
>> only uses update() API) with this patch and make it a single patch where
>> pruss_cfg_read() / update() is introduced as well as used, then this warning
>> will be resolved.
>>
> 
> The proper way to do this is to introduce new APIs only when they are needed.
> 

Sure, Mathieu. I will squash this patch with patch 5 ( as it uses both update()
and read() APIs) so that these APIs are introduced and used in the same patch.

>> I still think making these APIs "inline" is a better option as these APIs
>> implement very simple one line logic and can be made inline.
>>
>> Please let me know what do you think and which approach sounds better.
>>
>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index 126b672b9b30..2fa7df667592 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include <linux/regmap.h>
>>>>  #include <linux/remoteproc.h>
>>>>  #include <linux/slab.h>
>>>> +#include "pruss.h"
>>>>
>>>>  /**
>>>>   * struct pruss_private_data - PRUSS driver private data
>>>> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
>>>> new file mode 100644
>>>> index 000000000000..4626d5f6b874
>>>> --- /dev/null
>>>> +++ b/drivers/soc/ti/pruss.h
>>>> @@ -0,0 +1,112 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * PRU-ICSS Subsystem user interfaces
>>>> + *
>>>> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
>>>> + *  MD Danish Anwar <danishanwar@ti.com>
>>>> + */
>>>> +
>>>> +#ifndef _SOC_TI_PRUSS_H_
>>>> +#define _SOC_TI_PRUSS_H_
>>>> +
>>>> +#include <linux/bits.h>
>>>> +#include <linux/regmap.h>
>>>> +
>>>> +/*
>>>> + * 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)
>>>> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN              BIT(3)
>>>> +
>>>> +/**
>>>> + * 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);
>>>> +}
>>>> +
>>>> +#endif  /* _SOC_TI_PRUSS_H_ */
>>>> --
>>>> 2.25.1
>>>>
>>
>> --
>> Thanks and Regards,
>> Danish.

-- 
Thanks and Regards,
Danish.

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

[Md Danish Anwar]

Hi Mathieu,

On 31/03/23 15:52, Md Danish Anwar wrote:
> On 30/03/23 19:51, Mathieu Poirier wrote:
>> On Thu, 30 Mar 2023 at 04:00, Md Danish Anwar <a0501179@ti.com> wrote:
>>>
>>> Hi Mathieu,
>>>
>>> On 28/03/23 02:31, Mathieu Poirier wrote:
>>>> On Thu, Mar 23, 2023 at 11:54:49AM +0530, MD Danish Anwar wrote:
>>>>> From: Suman Anna <s-anna@ti.com>
>>>>>
>>>>> 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 <s-anna@ti.com>
>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> ---
>>>>>  drivers/soc/ti/pruss.c |   1 +
>>>>>  drivers/soc/ti/pruss.h | 112 +++++++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 113 insertions(+)
>>>>>  create mode 100644 drivers/soc/ti/pruss.h
>>>>>
>>>>
>>>> This patch doesn't compile without warnings.
>>>>
>>>
>>> I checked the warnings. Below are the warnings that I am getting for these patch.
>>>
>>> In file included from drivers/soc/ti/pruss.c:24:
>>> drivers/soc/ti/pruss.h:103:12: warning: ‘pruss_cfg_update’ defined but not used
>>> [-Wunused-function]
>>>   103 | static int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>       |            ^~~~~~~~~~~~~~~~
>>> drivers/soc/ti/pruss.h:84:12: warning: ‘pruss_cfg_read’ defined but not used
>>> [-Wunused-function]
>>>    84 | static int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
>>> unsigned int *val)
>>>
>>> These warnings are coming because pruss_cfg_read() / update() APIs are
>>> introduced in this patch but they are used later.
>>>
>>> One way to resolve this warning is to make this API "inline". I compiled after
>>> making these APIs inline, it got compiled without any warnings.
>>>
>>> The other solution is to merge a user API of these APIs in this patch. Patch 4
>>> and 5 introduces some APIs that uses pruss_cfg_read() / update() APIs. If we
>>> squash patch 5 (as patch 5 uses both read() and update() APIs where as patch 4
>>> only uses update() API) with this patch and make it a single patch where
>>> pruss_cfg_read() / update() is introduced as well as used, then this warning
>>> will be resolved.
>>>
>>
>> The proper way to do this is to introduce new APIs only when they are needed.
>>
> 
> Sure, Mathieu. I will squash this patch with patch 5 ( as it uses both update()
> and read() APIs) so that these APIs are introduced and used in the same patch.
> 

I have sent next revision [v6] of these patch-set addressing your comments.
Please have a look at that.

[v6] https://lore.kernel.org/all/20230331112941.823410-1-danishanwar@ti.com/

-- 
Thanks and Regards,
Danish.