LinuxLists.cc - [PATCH v3 0/6] Introduce PRU platform consumer API

[permalink] [raw]

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

From: Tero Kristo <[email protected]>

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

Co-developed-by: Suman Anna <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
Signed-off-by: Tero Kristo <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
---
include/linux/pruss_driver.h | 44 ++++++++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)

diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
index 22b4b37d2536..80b668889d9d 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_driver.h
@@ -35,4 +35,48 @@ struct pruss {
struct clk *iep_clk_mux;
};

+/**
+ * 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
+ */
+static inline 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;
+}
+
+/**
+ * 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
+ */
+static inline 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);
+}
+
#endif /* _PRUSS_DRIVER_H_ */
--
2.25.1

2023-03-06 11:10:15

[permalink] [raw]

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

From: Suman Anna <[email protected]>

Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
the PRUSS platform driver to allow other drivers to read and program
respectively a register within the PRUSS CFG sub-module represented
by a syscon driver. This interface provides a simple way for client
drivers without having them to include and parse the CFG syscon node
within their respective device nodes. Various useful registers and
macros for certain register bit-fields and their values have also
been added.

It is the responsibility of the client drivers to reconfigure or
reset a particular register upon any failures.

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

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

+/**
+ * pruss_cfg_read() - read a PRUSS CFG sub-module register
+ * @pruss: the pruss instance handle
+ * @reg: register offset within the CFG sub-module
+ * @val: pointer to return the value in
+ *
+ * Reads a given register within the PRUSS CFG sub-module and
+ * returns it through the passed-in @val pointer
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+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);
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_read);
+
+/**
+ * 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
+ */
+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);
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_update);
+
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..d41bec448f06 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -10,12 +10,99 @@
#ifndef __LINUX_PRUSS_H
#define __LINUX_PRUSS_H

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

#define PRU_RPROC_DRVNAME "pru-rproc"

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

#else

@@ -101,6 +191,18 @@ static inline int pruss_release_mem_region(struct pruss *pruss,
return -EOPNOTSUPP;
}

+static inline int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
+ unsigned int *val)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
+ unsigned int mask, unsigned int val)
+{
+ return -EOPNOTSUPP;
+}
+
#endif /* CONFIG_TI_PRUSS */

#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
--
2.25.1

2023-03-06 11:10:21

[permalink] [raw]

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

From: Suman Anna <[email protected]>

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

Signed-off-by: Suman Anna <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
---
include/linux/remoteproc/pruss.h | 55 ++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)

diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index d41bec448f06..7952f250301a 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -240,4 +240,59 @@ static inline bool is_pru_rproc(struct device *dev)
return true;
}

+/**
+ * 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
+ */
+static inline 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;
+
+ return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
+ PRUSS_GPCFG_PRU_GPI_MODE_MASK,
+ mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
+}
+
+/**
+ * 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
+ */
+static inline 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);
+}
+
+/**
+ * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
+ * @pruss: the pruss instance
+ * @enable: enable/disable
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable)
+{
+ u32 set = enable ? PRUSS_SPP_XFER_SHIFT_EN : 0;
+
+ return pruss_cfg_update(pruss, PRUSS_CFG_SPP,
+ PRUSS_SPP_XFER_SHIFT_EN, set);
+}
+
#endif /* __LINUX_PRUSS_H */
--
2.25.1

2023-03-06 11:10:23

[permalink] [raw]

Subject: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

From: Suman Anna <[email protected]>

The PRU-ICSS subsystem on OMAP-architecture based SoCS (AM33xx, AM437x
and AM57xx SoCs) has a control bit STANDBY_INIT in the PRUSS_CFG register
to initiate a Standby sequence (when set) and trigger a MStandby request
to the SoC's PRCM module. This same bit is also used to enable the OCP
master ports (when cleared). The clearing of the STANDBY_INIT bit requires
an acknowledgment from PRCM and is done through the monitoring of the
PRUSS_SYSCFG.SUB_MWAIT bit.

Add a helper function pruss_cfg_ocp_master_ports() to allow the PRU
client drivers to control this bit and enable or disable the firmware
running on PRU cores access to any peripherals or memory to achieve
desired functionality. The access is disabled by default on power-up
and on any suspend (context is not maintained).

Signed-off-by: Suman Anna <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
---
drivers/soc/ti/pruss.c | 81 +++++++++++++++++++++++++++++++-
include/linux/remoteproc/pruss.h | 6 +++
2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 537a3910ffd8..dc3abda0b8c2 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -22,14 +22,19 @@
#include <linux/remoteproc.h>
#include <linux/slab.h>

+#define SYSCFG_STANDBY_INIT BIT(4)
+#define SYSCFG_SUB_MWAIT_READY BIT(5)
+
/**
* struct pruss_private_data - PRUSS driver private data
* @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
* @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
+ * @has_ocp_syscfg: flag to indicate if OCP SYSCFG is present
*/
struct pruss_private_data {
bool has_no_sharedram;
bool has_core_mux_clock;
+ bool has_ocp_syscfg;
};

/**
@@ -205,6 +210,72 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
}
EXPORT_SYMBOL_GPL(pruss_cfg_update);

+/**
+ * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
+ * @pruss: the pruss instance handle
+ * @enable: set to true for enabling or false for disabling the OCP master ports
+ *
+ * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
+ * disable the OCP master ports (applicable only on SoCs using OCP interconnect
+ * like the OMAP family). Clearing the bit achieves dual functionalities - one
+ * is to deassert the MStandby signal to the device PRCM, and the other is to
+ * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
+ * function has to wait for the PRCM to acknowledge through the monitoring of
+ * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
+ * disables the master access, and also signals the PRCM that the PRUSS is ready
+ * for Standby.
+ *
+ * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
+ * when the ready-state fails.
+ */
+int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
+{
+ int ret;
+ u32 syscfg_val, i;
+ const struct pruss_private_data *data;
+
+ if (IS_ERR_OR_NULL(pruss))
+ return -EINVAL;
+
+ data = of_device_get_match_data(pruss->dev);
+
+ /* nothing to do on non OMAP-SoCs */
+ if (!data || !data->has_ocp_syscfg)
+ return 0;
+
+ /* assert the MStandby signal during disable path */
+ if (!enable)
+ return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
+ SYSCFG_STANDBY_INIT,
+ SYSCFG_STANDBY_INIT);
+
+ /* enable the OCP master ports and disable MStandby */
+ ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
+ if (ret)
+ return ret;
+
+ /* wait till we are ready for transactions - delay is arbitrary */
+ for (i = 0; i < 10; i++) {
+ ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
+ if (ret)
+ goto disable;
+
+ if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
+ return 0;
+
+ udelay(5);
+ }
+
+ dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
+ ret = -ETIMEDOUT;
+
+disable:
+ pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
+ SYSCFG_STANDBY_INIT);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
+
static void pruss_of_free_clk_provider(void *data)
{
struct device_node *clk_mux_np = data;
@@ -495,10 +566,16 @@ static int pruss_remove(struct platform_device *pdev)
/* instance-specific driver private data */
static const struct pruss_private_data am437x_pruss1_data = {
.has_no_sharedram = false,
+ .has_ocp_syscfg = true,
};

static const struct pruss_private_data am437x_pruss0_data = {
.has_no_sharedram = true,
+ .has_ocp_syscfg = false,
+};
+
+static const struct pruss_private_data am33xx_am57xx_data = {
+ .has_ocp_syscfg = true,
};

static const struct pruss_private_data am65x_j721e_pruss_data = {
@@ -506,10 +583,10 @@ static const struct pruss_private_data am65x_j721e_pruss_data = {
};

static const struct of_device_id pruss_of_match[] = {
- { .compatible = "ti,am3356-pruss" },
+ { .compatible = "ti,am3356-pruss", .data = &am33xx_am57xx_data },
{ .compatible = "ti,am4376-pruss0", .data = &am437x_pruss0_data, },
{ .compatible = "ti,am4376-pruss1", .data = &am437x_pruss1_data, },
- { .compatible = "ti,am5728-pruss" },
+ { .compatible = "ti,am5728-pruss", .data = &am33xx_am57xx_data },
{ .compatible = "ti,k2g-pruss" },
{ .compatible = "ti,am654-icssg", .data = &am65x_j721e_pruss_data, },
{ .compatible = "ti,j721e-icssg", .data = &am65x_j721e_pruss_data, },
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 7952f250301a..8cb99d3cad0d 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -168,6 +168,7 @@ int pruss_release_mem_region(struct pruss *pruss,
int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
unsigned int mask, unsigned int val);
+int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable);

#else

@@ -203,6 +204,11 @@ static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
return -EOPNOTSUPP;
}

+static inline int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
+{
+ return -EOPNOTSUPP;
+}
+
#endif /* CONFIG_TI_PRUSS */

#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
--
2.25.1

2023-03-06 18:44:33

by Mathieu Poirier

[permalink] [raw]

Subject: Re: [PATCH v3 0/6] Introduce PRU platform consumer API

On Mon, Mar 06, 2023 at 04:39:28PM +0530, 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.
>
> This is the v3 of the old patch series[1]. This doesn't have any 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[2] 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 [3],which
> depends on this series and the remoteproc series[2].
>
> [1] https://lore.kernel.org/all/[email protected]/
> [2] https://lore.kernel.org/all/[email protected]/#t
> [3] https://lore.kernel.org/all/[email protected]/
>
> Thanks and Regards,
> Md Danish Anwar
>
> Andrew F. Davis (1):
> soc: ti: pruss: Add pruss_{request,release}_mem_region() API
>
> Suman Anna (3):
> soc: ti: pruss: Add pruss_cfg_read()/update() API
> soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and
> XFR
> soc: ti: pruss: Add helper function to enable OCP master ports
>
> Tero Kristo (2):
> soc: ti: pruss: Add pruss_get()/put() API
> soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX
>
> drivers/soc/ti/pruss.c | 257 ++++++++++++++++++++++++++++++-
> include/linux/pruss_driver.h | 72 ++++++---
> include/linux/remoteproc/pruss.h | 221 ++++++++++++++++++++++++++
> 3 files changed, 526 insertions(+), 24 deletions(-)

The last revision of this set was sent out on April 18th 2022... It is always
very difficult to follow-up with a patchset when it has been this long.
Moreover, you added a SoB to patch 1 and 2 but none of the other ones.

Roger had comments on the previous set - I will look at this revision when he
has provided his RB for this entire set.

Thanks,
Mathieu

>
> --
> 2.25.1
>

2023-03-08 08:30:27

[permalink] [raw]

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

Hi,

On 06/03/2023 13:09, MD Danish Anwar wrote:
> From: Suman Anna <[email protected]>
>
> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
> the PRUSS platform driver to allow other drivers to read and program
> respectively a register within the PRUSS CFG sub-module represented
> by a syscon driver. This interface provides a simple way for client

Do you really need these 2 functions to be public?
I see that later patches (4-6) add APIs for doing specific things
and that should be sufficient than exposing entire CFG space via
pruss_cfg_read/update().

> drivers without having them to include and parse the CFG syscon node
> within their respective device nodes. Various useful registers and
> macros for certain register bit-fields and their values have also
> been added.
>
> It is the responsibility of the client drivers to reconfigure or
> reset a particular register upon any failures.
>
> Signed-off-by: Suman Anna <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>
> ---
> drivers/soc/ti/pruss.c | 41 +++++++++++++
> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
> 2 files changed, 143 insertions(+)
>
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index c8053c0d735f..537a3910ffd8 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
> }
> EXPORT_SYMBOL_GPL(pruss_release_mem_region);

cheers,
-roger

2023-03-08 08:36:38

[permalink] [raw]

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

Hi Danish,

On 06/03/2023 13:09, MD Danish Anwar wrote:
> From: Suman Anna <[email protected]>
>
> The PRUSS CFG module is represented as a syscon node and is currently
> managed by the PRUSS platform driver. Add easy accessor functions to set
> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
> to enable the PRUSS Ethernet usecase. These functions reuse the generic
> pruss_cfg_update() API function.
>
> Signed-off-by: Suman Anna <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>
> ---
> include/linux/remoteproc/pruss.h | 55 ++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> index d41bec448f06..7952f250301a 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -240,4 +240,59 @@ static inline bool is_pru_rproc(struct device *dev)
> return true;
> }
>
> +/**
> + * 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
> + */
> +static inline 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;
> +

Should we check for invalid gpi mode and error out if so?

> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
> + PRUSS_GPCFG_PRU_GPI_MODE_MASK,
> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT);
> +}
> +
> +/**
> + * 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
> + */
> +static inline 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);
> +}
> +
> +/**
> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
> + * @pruss: the pruss instance
> + * @enable: enable/disable
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable)
> +{
> + u32 set = enable ? PRUSS_SPP_XFER_SHIFT_EN : 0;
> +
> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP,
> + PRUSS_SPP_XFER_SHIFT_EN, set);
> +}
> +
> #endif /* __LINUX_PRUSS_H */

cheers,
-roger

2023-03-08 08:42:23

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

On 06/03/2023 13:09, MD Danish Anwar wrote:
> From: Suman Anna <[email protected]>
>
> The PRU-ICSS subsystem on OMAP-architecture based SoCS (AM33xx, AM437x
> and AM57xx SoCs) has a control bit STANDBY_INIT in the PRUSS_CFG register
> to initiate a Standby sequence (when set) and trigger a MStandby request
> to the SoC's PRCM module. This same bit is also used to enable the OCP
> master ports (when cleared). The clearing of the STANDBY_INIT bit requires
> an acknowledgment from PRCM and is done through the monitoring of the
> PRUSS_SYSCFG.SUB_MWAIT bit.
>
> Add a helper function pruss_cfg_ocp_master_ports() to allow the PRU
> client drivers to control this bit and enable or disable the firmware
> running on PRU cores access to any peripherals or memory to achieve
> desired functionality. The access is disabled by default on power-up
> and on any suspend (context is not maintained).
>
> Signed-off-by: Suman Anna <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>
> ---
> drivers/soc/ti/pruss.c | 81 +++++++++++++++++++++++++++++++-
> include/linux/remoteproc/pruss.h | 6 +++
> 2 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 537a3910ffd8..dc3abda0b8c2 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -22,14 +22,19 @@
> #include <linux/remoteproc.h>
> #include <linux/slab.h>
>
> +#define SYSCFG_STANDBY_INIT BIT(4)
> +#define SYSCFG_SUB_MWAIT_READY BIT(5)
> +
> /**
> * struct pruss_private_data - PRUSS driver private data
> * @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
> * @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
> + * @has_ocp_syscfg: flag to indicate if OCP SYSCFG is present
> */
> struct pruss_private_data {
> bool has_no_sharedram;
> bool has_core_mux_clock;
> + bool has_ocp_syscfg;
> };
>
> /**
> @@ -205,6 +210,72 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
> }
> EXPORT_SYMBOL_GPL(pruss_cfg_update);
>
> +/**
> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
> + * @pruss: the pruss instance handle
> + * @enable: set to true for enabling or false for disabling the OCP master ports
> + *
> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
> + * is to deassert the MStandby signal to the device PRCM, and the other is to
> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
> + * function has to wait for the PRCM to acknowledge through the monitoring of
> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
> + * disables the master access, and also signals the PRCM that the PRUSS is ready
> + * for Standby.
> + *
> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
> + * when the ready-state fails.
> + */
> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
> +{
> + int ret;
> + u32 syscfg_val, i;
> + const struct pruss_private_data *data;
> +
> + if (IS_ERR_OR_NULL(pruss))
> + return -EINVAL;
> +
> + data = of_device_get_match_data(pruss->dev);
> +
> + /* nothing to do on non OMAP-SoCs */
> + if (!data || !data->has_ocp_syscfg)
> + return 0;
> +
> + /* assert the MStandby signal during disable path */
> + if (!enable)
> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
> + SYSCFG_STANDBY_INIT,
> + SYSCFG_STANDBY_INIT);

You can omit the above if() if you just encapsulate the below in

if (enable) {

> +
> + /* enable the OCP master ports and disable MStandby */
> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
> + if (ret)
> + return ret;
> +
> + /* wait till we are ready for transactions - delay is arbitrary */
> + for (i = 0; i < 10; i++) {
> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
> + if (ret)
> + goto disable;
> +
> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
> + return 0;
> +
> + udelay(5);
> + }
> +
> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
> + ret = -ETIMEDOUT;

}

> +
> +disable:
> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
> + SYSCFG_STANDBY_INIT);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
> +
> static void pruss_of_free_clk_provider(void *data)
> {
> struct device_node *clk_mux_np = data;
> @@ -495,10 +566,16 @@ static int pruss_remove(struct platform_device *pdev)
> /* instance-specific driver private data */
> static const struct pruss_private_data am437x_pruss1_data = {
> .has_no_sharedram = false,
> + .has_ocp_syscfg = true,
> };
>
> static const struct pruss_private_data am437x_pruss0_data = {
> .has_no_sharedram = true,
> + .has_ocp_syscfg = false,
> +};
> +
> +static const struct pruss_private_data am33xx_am57xx_data = {
> + .has_ocp_syscfg = true,
> };

How about keeping platform data for different platforms separate?

i.e. am33xx_pruss_data and am57xx_pruss_data

>
> static const struct pruss_private_data am65x_j721e_pruss_data = {
> @@ -506,10 +583,10 @@ static const struct pruss_private_data am65x_j721e_pruss_data = {
> };
>
> static const struct of_device_id pruss_of_match[] = {
> - { .compatible = "ti,am3356-pruss" },
> + { .compatible = "ti,am3356-pruss", .data = &am33xx_am57xx_data },
> { .compatible = "ti,am4376-pruss0", .data = &am437x_pruss0_data, },
> { .compatible = "ti,am4376-pruss1", .data = &am437x_pruss1_data, },
> - { .compatible = "ti,am5728-pruss" },
> + { .compatible = "ti,am5728-pruss", .data = &am33xx_am57xx_data },
> { .compatible = "ti,k2g-pruss" },
> { .compatible = "ti,am654-icssg", .data = &am65x_j721e_pruss_data, },
> { .compatible = "ti,j721e-icssg", .data = &am65x_j721e_pruss_data, },
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> index 7952f250301a..8cb99d3cad0d 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -168,6 +168,7 @@ int pruss_release_mem_region(struct pruss *pruss,
> int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
> int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
> unsigned int mask, unsigned int val);
> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable);
>
> #else
>
> @@ -203,6 +204,11 @@ static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
> return -EOPNOTSUPP;
> }
>
> +static inline int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #endif /* CONFIG_TI_PRUSS */
>
> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)

cheers,
-roger

2023-03-08 08:43:30

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

On 06/03/2023 13:09, MD Danish Anwar wrote:
> From: Tero Kristo <[email protected]>
>
> Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
> to get and set the GP MUX mode for programming the PRUSS internal wrapper
> mux functionality as needed by usecases.
>
> Co-developed-by: Suman Anna <[email protected]>
> Signed-off-by: Suman Anna <[email protected]>
> Signed-off-by: Tero Kristo <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>

Reviewed-by: Roger Quadros <[email protected]>

2023-03-08 09:25:10

[permalink] [raw]

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

Hi Roger,

On 08/03/23 14:04, Roger Quadros wrote:
> Hi Danish,
>
> On 06/03/2023 13:09, MD Danish Anwar wrote:
>> From: Suman Anna <[email protected]>
>>
>> The PRUSS CFG module is represented as a syscon node and is currently
>> managed by the PRUSS platform driver. Add easy accessor functions to set
>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>> pruss_cfg_update() API function.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> ---
>> include/linux/remoteproc/pruss.h | 55 ++++++++++++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>>
>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>> index d41bec448f06..7952f250301a 100644
>> --- a/include/linux/remoteproc/pruss.h
>> +++ b/include/linux/remoteproc/pruss.h
>> @@ -240,4 +240,59 @@ static inline bool is_pru_rproc(struct device *dev)
>> return true;
>> }
>>
>> +/**
>> + * 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
>> + */
>> +static inline 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;
>> +
>
> Should we check for invalid gpi mode and error out if so?
>
Sure we can check for invalid gpi mode.

Does the below code snippet looks good to you?

if(mode < PRUSS_GPI_MODE_DIRECT || mode > PRUSS_GPI_MODE_MII)
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);
>> +}
>> +
>> +/**
>> + * 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
>> + */
>> +static inline 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);
>> +}
>> +
>> +/**
>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>> + * @pruss: the pruss instance
>> + * @enable: enable/disable
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable)
>> +{
>> + u32 set = enable ? PRUSS_SPP_XFER_SHIFT_EN : 0;
>> +
>> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP,
>> + PRUSS_SPP_XFER_SHIFT_EN, set);
>> +}
>> +
>> #endif /* __LINUX_PRUSS_H */
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-08 11:10:24

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

Hi Roger,

On 08/03/23 14:11, Roger Quadros wrote:
>
>
> On 06/03/2023 13:09, MD Danish Anwar wrote:
>> From: Suman Anna <[email protected]>
>>
>> The PRU-ICSS subsystem on OMAP-architecture based SoCS (AM33xx, AM437x
>> and AM57xx SoCs) has a control bit STANDBY_INIT in the PRUSS_CFG register
>> to initiate a Standby sequence (when set) and trigger a MStandby request
>> to the SoC's PRCM module. This same bit is also used to enable the OCP
>> master ports (when cleared). The clearing of the STANDBY_INIT bit requires
>> an acknowledgment from PRCM and is done through the monitoring of the
>> PRUSS_SYSCFG.SUB_MWAIT bit.
>>
>> Add a helper function pruss_cfg_ocp_master_ports() to allow the PRU
>> client drivers to control this bit and enable or disable the firmware
>> running on PRU cores access to any peripherals or memory to achieve
>> desired functionality. The access is disabled by default on power-up
>> and on any suspend (context is not maintained).
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> ---
>> drivers/soc/ti/pruss.c | 81 +++++++++++++++++++++++++++++++-
>> include/linux/remoteproc/pruss.h | 6 +++
>> 2 files changed, 85 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 537a3910ffd8..dc3abda0b8c2 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -22,14 +22,19 @@
>> #include <linux/remoteproc.h>
>> #include <linux/slab.h>
>>
>> +#define SYSCFG_STANDBY_INIT BIT(4)
>> +#define SYSCFG_SUB_MWAIT_READY BIT(5)
>> +
>> /**
>> * struct pruss_private_data - PRUSS driver private data
>> * @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
>> * @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
>> + * @has_ocp_syscfg: flag to indicate if OCP SYSCFG is present
>> */
>> struct pruss_private_data {
>> bool has_no_sharedram;
>> bool has_core_mux_clock;
>> + bool has_ocp_syscfg;
>> };
>>
>> /**
>> @@ -205,6 +210,72 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>> }
>> EXPORT_SYMBOL_GPL(pruss_cfg_update);
>>
>> +/**
>> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
>> + * @pruss: the pruss instance handle
>> + * @enable: set to true for enabling or false for disabling the OCP master ports
>> + *
>> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
>> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
>> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
>> + * is to deassert the MStandby signal to the device PRCM, and the other is to
>> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
>> + * function has to wait for the PRCM to acknowledge through the monitoring of
>> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
>> + * disables the master access, and also signals the PRCM that the PRUSS is ready
>> + * for Standby.
>> + *
>> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
>> + * when the ready-state fails.
>> + */
>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>> +{
>> + int ret;
>> + u32 syscfg_val, i;
>> + const struct pruss_private_data *data;
>> +
>> + if (IS_ERR_OR_NULL(pruss))
>> + return -EINVAL;
>> +
>> + data = of_device_get_match_data(pruss->dev);
>> +
>> + /* nothing to do on non OMAP-SoCs */
>> + if (!data || !data->has_ocp_syscfg)
>> + return 0;
>> +
>> + /* assert the MStandby signal during disable path */
>> + if (!enable)
>> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
>> + SYSCFG_STANDBY_INIT,
>> + SYSCFG_STANDBY_INIT);
>
> You can omit the above if() if you just encapsulate the below in
>
> if (enable) {
>
>
Sure, I can omit the above if() and put the below block inside if (enable) {}.

Currently when API pruss_cfg_ocp_master_ports()is called with enable as false
i.e. disabling PRUSS OCP master ports is requested, we directly return
pruss_cfg_update() where as if we remove the above if() section and encapsulate
below block in if (enable) {}, then in disable scenario, call flow will
directly reach the label disable. In the label disable, we are updating cfg and
then returning "ret", but at this point the variable ret is not assigned.

To counter this should I change the label disable to below?

disable:
return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
SYSCFG_STANDBY_INIT);

>> +
>> + /* enable the OCP master ports and disable MStandby */
>> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
>> + if (ret)
>> + return ret;
>> +
>> + /* wait till we are ready for transactions - delay is arbitrary */
>> + for (i = 0; i < 10; i++) {
>> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
>> + if (ret)
>> + goto disable;
>> +

Changing the disable label will also result in losing the return value of
pruss_cfg_read() API call here.

>> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
>> + return 0;
>> +
>> + udelay(5);
>> + }
>> +
>> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
>> + ret = -ETIMEDOUT;

Changing the disable label will also result in losing ret = -ETIMEDOUT here.

>
> }
>
>> +
>> +disable:
>> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>> + SYSCFG_STANDBY_INIT);
>> + return ret;
>> +}

So should I do this modification or keep it as it is?

>> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
>> +
>> static void pruss_of_free_clk_provider(void *data)
>> {
>> struct device_node *clk_mux_np = data;
>> @@ -495,10 +566,16 @@ static int pruss_remove(struct platform_device *pdev)
>> /* instance-specific driver private data */
>> static const struct pruss_private_data am437x_pruss1_data = {
>> .has_no_sharedram = false,
>> + .has_ocp_syscfg = true,
>> };
>>
>> static const struct pruss_private_data am437x_pruss0_data = {
>> .has_no_sharedram = true,
>> + .has_ocp_syscfg = false,
>> +};
>> +
>> +static const struct pruss_private_data am33xx_am57xx_data = {
>> + .has_ocp_syscfg = true,
>> };
>
> How about keeping platform data for different platforms separate?
>
> i.e. am33xx_pruss_data and am57xx_pruss_data
>

Sure. I'll split am33xx_am57xx_data into am33xx_pruss_data and
am57xx_pruss_data as well as am65x_j721e_pruss_data into am65x_pruss_data and
j721e_pruss_data.

>>
>> static const struct pruss_private_data am65x_j721e_pruss_data = {
>> @@ -506,10 +583,10 @@ static const struct pruss_private_data am65x_j721e_pruss_data = {
>> };
>>
>> static const struct of_device_id pruss_of_match[] = {
>> - { .compatible = "ti,am3356-pruss" },
>> + { .compatible = "ti,am3356-pruss", .data = &am33xx_am57xx_data },
>> { .compatible = "ti,am4376-pruss0", .data = &am437x_pruss0_data, },
>> { .compatible = "ti,am4376-pruss1", .data = &am437x_pruss1_data, },
>> - { .compatible = "ti,am5728-pruss" },
>> + { .compatible = "ti,am5728-pruss", .data = &am33xx_am57xx_data },
>> { .compatible = "ti,k2g-pruss" },
>> { .compatible = "ti,am654-icssg", .data = &am65x_j721e_pruss_data, },
>> { .compatible = "ti,j721e-icssg", .data = &am65x_j721e_pruss_data, },
>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>> index 7952f250301a..8cb99d3cad0d 100644
>> --- a/include/linux/remoteproc/pruss.h
>> +++ b/include/linux/remoteproc/pruss.h
>> @@ -168,6 +168,7 @@ int pruss_release_mem_region(struct pruss *pruss,
>> int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
>> int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>> unsigned int mask, unsigned int val);
>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable);
>>
>> #else
>>
>> @@ -203,6 +204,11 @@ static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>> return -EOPNOTSUPP;
>> }
>>
>> +static inline int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> #endif /* CONFIG_TI_PRUSS */
>>
>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-08 11:14:47

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

On 08/03/2023 13:09, Md Danish Anwar wrote:
> Hi Roger,
>
> On 08/03/23 14:11, Roger Quadros wrote:
>>
>>
>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>> From: Suman Anna <[email protected]>
>>>
>>> The PRU-ICSS subsystem on OMAP-architecture based SoCS (AM33xx, AM437x
>>> and AM57xx SoCs) has a control bit STANDBY_INIT in the PRUSS_CFG register
>>> to initiate a Standby sequence (when set) and trigger a MStandby request
>>> to the SoC's PRCM module. This same bit is also used to enable the OCP
>>> master ports (when cleared). The clearing of the STANDBY_INIT bit requires
>>> an acknowledgment from PRCM and is done through the monitoring of the
>>> PRUSS_SYSCFG.SUB_MWAIT bit.
>>>
>>> Add a helper function pruss_cfg_ocp_master_ports() to allow the PRU
>>> client drivers to control this bit and enable or disable the firmware
>>> running on PRU cores access to any peripherals or memory to achieve
>>> desired functionality. The access is disabled by default on power-up
>>> and on any suspend (context is not maintained).
>>>
>>> Signed-off-by: Suman Anna <[email protected]>
>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> ---
>>> drivers/soc/ti/pruss.c | 81 +++++++++++++++++++++++++++++++-
>>> include/linux/remoteproc/pruss.h | 6 +++
>>> 2 files changed, 85 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>> index 537a3910ffd8..dc3abda0b8c2 100644
>>> --- a/drivers/soc/ti/pruss.c
>>> +++ b/drivers/soc/ti/pruss.c
>>> @@ -22,14 +22,19 @@
>>> #include <linux/remoteproc.h>
>>> #include <linux/slab.h>
>>>
>>> +#define SYSCFG_STANDBY_INIT BIT(4)
>>> +#define SYSCFG_SUB_MWAIT_READY BIT(5)
>>> +
>>> /**
>>> * struct pruss_private_data - PRUSS driver private data
>>> * @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
>>> * @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
>>> + * @has_ocp_syscfg: flag to indicate if OCP SYSCFG is present
>>> */
>>> struct pruss_private_data {
>>> bool has_no_sharedram;
>>> bool has_core_mux_clock;
>>> + bool has_ocp_syscfg;
>>> };
>>>
>>> /**
>>> @@ -205,6 +210,72 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>> }
>>> EXPORT_SYMBOL_GPL(pruss_cfg_update);
>>>
>>> +/**
>>> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
>>> + * @pruss: the pruss instance handle
>>> + * @enable: set to true for enabling or false for disabling the OCP master ports
>>> + *
>>> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
>>> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
>>> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
>>> + * is to deassert the MStandby signal to the device PRCM, and the other is to
>>> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
>>> + * function has to wait for the PRCM to acknowledge through the monitoring of
>>> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
>>> + * disables the master access, and also signals the PRCM that the PRUSS is ready
>>> + * for Standby.
>>> + *
>>> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
>>> + * when the ready-state fails.
>>> + */
>>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>>> +{
>>> + int ret;
>>> + u32 syscfg_val, i;
>>> + const struct pruss_private_data *data;
>>> +
>>> + if (IS_ERR_OR_NULL(pruss))
>>> + return -EINVAL;
>>> +
>>> + data = of_device_get_match_data(pruss->dev);
>>> +
>>> + /* nothing to do on non OMAP-SoCs */
>>> + if (!data || !data->has_ocp_syscfg)
>>> + return 0;
>>> +
>>> + /* assert the MStandby signal during disable path */
>>> + if (!enable)
>>> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
>>> + SYSCFG_STANDBY_INIT,
>>> + SYSCFG_STANDBY_INIT);
>>
>> You can omit the above if() if you just encapsulate the below in
>>
>> if (enable) {
>>
>>
> Sure, I can omit the above if() and put the below block inside if (enable) {}.
>
> Currently when API pruss_cfg_ocp_master_ports()is called with enable as false
> i.e. disabling PRUSS OCP master ports is requested, we directly return
> pruss_cfg_update() where as if we remove the above if() section and encapsulate
> below block in if (enable) {}, then in disable scenario, call flow will
> directly reach the label disable. In the label disable, we are updating cfg and
> then returning "ret", but at this point the variable ret is not assigned.
>
> To counter this should I change the label disable to below?
>
> disable:
> return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
> SYSCFG_STANDBY_INIT);

But you will loose the error code if we came here due to failure in pruss_cfg_read().

It's ok, don't make the change I suggested and leave it as it is.

>
>>> +
>>> + /* enable the OCP master ports and disable MStandby */
>>> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* wait till we are ready for transactions - delay is arbitrary */
>>> + for (i = 0; i < 10; i++) {
>>> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
>>> + if (ret)
>>> + goto disable;
>>> +
>
> Changing the disable label will also result in losing the return value of
> pruss_cfg_read() API call here.
>
>>> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
>>> + return 0;
>>> +
>>> + udelay(5);
>>> + }
>>> +
>>> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
>>> + ret = -ETIMEDOUT;
>
> Changing the disable label will also result in losing ret = -ETIMEDOUT here.
>
>>
>> }
>>
>>> +
>>> +disable:
>>> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>>> + SYSCFG_STANDBY_INIT);
>>> + return ret;
>>> +}
>
> So should I do this modification or keep it as it is?
>
>>> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
>>> +
>>> static void pruss_of_free_clk_provider(void *data)
>>> {
>>> struct device_node *clk_mux_np = data;
>>> @@ -495,10 +566,16 @@ static int pruss_remove(struct platform_device *pdev)
>>> /* instance-specific driver private data */
>>> static const struct pruss_private_data am437x_pruss1_data = {
>>> .has_no_sharedram = false,
>>> + .has_ocp_syscfg = true,
>>> };
>>>
>>> static const struct pruss_private_data am437x_pruss0_data = {
>>> .has_no_sharedram = true,
>>> + .has_ocp_syscfg = false,
>>> +};
>>> +
>>> +static const struct pruss_private_data am33xx_am57xx_data = {
>>> + .has_ocp_syscfg = true,
>>> };
>>
>> How about keeping platform data for different platforms separate?
>>
>> i.e. am33xx_pruss_data and am57xx_pruss_data
>>
>
> Sure. I'll split am33xx_am57xx_data into am33xx_pruss_data and
> am57xx_pruss_data as well as am65x_j721e_pruss_data into am65x_pruss_data and
> j721e_pruss_data.
>
>>>
>>> static const struct pruss_private_data am65x_j721e_pruss_data = {
>>> @@ -506,10 +583,10 @@ static const struct pruss_private_data am65x_j721e_pruss_data = {
>>> };
>>>
>>> static const struct of_device_id pruss_of_match[] = {
>>> - { .compatible = "ti,am3356-pruss" },
>>> + { .compatible = "ti,am3356-pruss", .data = &am33xx_am57xx_data },
>>> { .compatible = "ti,am4376-pruss0", .data = &am437x_pruss0_data, },
>>> { .compatible = "ti,am4376-pruss1", .data = &am437x_pruss1_data, },
>>> - { .compatible = "ti,am5728-pruss" },
>>> + { .compatible = "ti,am5728-pruss", .data = &am33xx_am57xx_data },
>>> { .compatible = "ti,k2g-pruss" },
>>> { .compatible = "ti,am654-icssg", .data = &am65x_j721e_pruss_data, },
>>> { .compatible = "ti,j721e-icssg", .data = &am65x_j721e_pruss_data, },
>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>> index 7952f250301a..8cb99d3cad0d 100644
>>> --- a/include/linux/remoteproc/pruss.h
>>> +++ b/include/linux/remoteproc/pruss.h
>>> @@ -168,6 +168,7 @@ int pruss_release_mem_region(struct pruss *pruss,
>>> int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
>>> int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>> unsigned int mask, unsigned int val);
>>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable);
>>>
>>> #else
>>>
>>> @@ -203,6 +204,11 @@ static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>> return -EOPNOTSUPP;
>>> }
>>>
>>> +static inline int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>>> +{
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> #endif /* CONFIG_TI_PRUSS */
>>>
>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>
>> cheers,
>> -roger
>

cheers,
-roger

2023-03-08 11:15:44

[permalink] [raw]

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

On 08/03/2023 11:23, Md Danish Anwar wrote:
> Hi Roger,
>
> On 08/03/23 14:04, Roger Quadros wrote:
>> Hi Danish,
>>
>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>> From: Suman Anna <[email protected]>
>>>
>>> The PRUSS CFG module is represented as a syscon node and is currently
>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>> pruss_cfg_update() API function.
>>>
>>> Signed-off-by: Suman Anna <[email protected]>
>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> ---
>>> include/linux/remoteproc/pruss.h | 55 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 55 insertions(+)
>>>
>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>> index d41bec448f06..7952f250301a 100644
>>> --- a/include/linux/remoteproc/pruss.h
>>> +++ b/include/linux/remoteproc/pruss.h
>>> @@ -240,4 +240,59 @@ static inline bool is_pru_rproc(struct device *dev)
>>> return true;
>>> }
>>>
>>> +/**
>>> + * 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
>>> + */
>>> +static inline 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;
>>> +
>>
>> Should we check for invalid gpi mode and error out if so?
>>
> Sure we can check for invalid gpi mode.
>
> Does the below code snippet looks good to you?
>
> if(mode < PRUSS_GPI_MODE_DIRECT || mode > PRUSS_GPI_MODE_MII)

How about?
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);
>>> +}
>>> +
>>> +/**
>>> + * 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
>>> + */
>>> +static inline 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);
>>> +}
>>> +
>>> +/**
>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>> + * @pruss: the pruss instance
>>> + * @enable: enable/disable
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable)
>>> +{
>>> + u32 set = enable ? PRUSS_SPP_XFER_SHIFT_EN : 0;
>>> +
>>> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP,
>>> + PRUSS_SPP_XFER_SHIFT_EN, set);
>>> +}
>>> +
>>> #endif /* __LINUX_PRUSS_H */
>>

cheers,
-roger

2023-03-08 11:17:15

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

On 08/03/23 16:44, Roger Quadros wrote:
>
>
> On 08/03/2023 13:09, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 08/03/23 14:11, Roger Quadros wrote:
>>>
>>>
>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>> From: Suman Anna <[email protected]>
>>>>
>>>> The PRU-ICSS subsystem on OMAP-architecture based SoCS (AM33xx, AM437x
>>>> and AM57xx SoCs) has a control bit STANDBY_INIT in the PRUSS_CFG register
>>>> to initiate a Standby sequence (when set) and trigger a MStandby request
>>>> to the SoC's PRCM module. This same bit is also used to enable the OCP
>>>> master ports (when cleared). The clearing of the STANDBY_INIT bit requires
>>>> an acknowledgment from PRCM and is done through the monitoring of the
>>>> PRUSS_SYSCFG.SUB_MWAIT bit.
>>>>
>>>> Add a helper function pruss_cfg_ocp_master_ports() to allow the PRU
>>>> client drivers to control this bit and enable or disable the firmware
>>>> running on PRU cores access to any peripherals or memory to achieve
>>>> desired functionality. The access is disabled by default on power-up
>>>> and on any suspend (context is not maintained).
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>> ---
>>>> drivers/soc/ti/pruss.c | 81 +++++++++++++++++++++++++++++++-
>>>> include/linux/remoteproc/pruss.h | 6 +++
>>>> 2 files changed, 85 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index 537a3910ffd8..dc3abda0b8c2 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -22,14 +22,19 @@
>>>> #include <linux/remoteproc.h>
>>>> #include <linux/slab.h>
>>>>
>>>> +#define SYSCFG_STANDBY_INIT BIT(4)
>>>> +#define SYSCFG_SUB_MWAIT_READY BIT(5)
>>>> +
>>>> /**
>>>> * struct pruss_private_data - PRUSS driver private data
>>>> * @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
>>>> * @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
>>>> + * @has_ocp_syscfg: flag to indicate if OCP SYSCFG is present
>>>> */
>>>> struct pruss_private_data {
>>>> bool has_no_sharedram;
>>>> bool has_core_mux_clock;
>>>> + bool has_ocp_syscfg;
>>>> };
>>>>
>>>> /**
>>>> @@ -205,6 +210,72 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> }
>>>> EXPORT_SYMBOL_GPL(pruss_cfg_update);
>>>>
>>>> +/**
>>>> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
>>>> + * @pruss: the pruss instance handle
>>>> + * @enable: set to true for enabling or false for disabling the OCP master ports
>>>> + *
>>>> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
>>>> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
>>>> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
>>>> + * is to deassert the MStandby signal to the device PRCM, and the other is to
>>>> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
>>>> + * function has to wait for the PRCM to acknowledge through the monitoring of
>>>> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
>>>> + * disables the master access, and also signals the PRCM that the PRUSS is ready
>>>> + * for Standby.
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
>>>> + * when the ready-state fails.
>>>> + */
>>>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>>>> +{
>>>> + int ret;
>>>> + u32 syscfg_val, i;
>>>> + const struct pruss_private_data *data;
>>>> +
>>>> + if (IS_ERR_OR_NULL(pruss))
>>>> + return -EINVAL;
>>>> +
>>>> + data = of_device_get_match_data(pruss->dev);
>>>> +
>>>> + /* nothing to do on non OMAP-SoCs */
>>>> + if (!data || !data->has_ocp_syscfg)
>>>> + return 0;
>>>> +
>>>> + /* assert the MStandby signal during disable path */
>>>> + if (!enable)
>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
>>>> + SYSCFG_STANDBY_INIT,
>>>> + SYSCFG_STANDBY_INIT);
>>>
>>> You can omit the above if() if you just encapsulate the below in
>>>
>>> if (enable) {
>>>
>>>
>> Sure, I can omit the above if() and put the below block inside if (enable) {}.
>>
>> Currently when API pruss_cfg_ocp_master_ports()is called with enable as false
>> i.e. disabling PRUSS OCP master ports is requested, we directly return
>> pruss_cfg_update() where as if we remove the above if() section and encapsulate
>> below block in if (enable) {}, then in disable scenario, call flow will
>> directly reach the label disable. In the label disable, we are updating cfg and
>> then returning "ret", but at this point the variable ret is not assigned.
>>
>> To counter this should I change the label disable to below?
>>
>> disable:
>> return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>> SYSCFG_STANDBY_INIT);
>
> But you will loose the error code if we came here due to failure in pruss_cfg_read().
>

Yes that's why I think it's better to leave it as it is.

> It's ok, don't make the change I suggested and leave it as it is.
>

Sure

>>
>>>> +
>>>> + /* enable the OCP master ports and disable MStandby */
>>>> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /* wait till we are ready for transactions - delay is arbitrary */
>>>> + for (i = 0; i < 10; i++) {
>>>> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
>>>> + if (ret)
>>>> + goto disable;
>>>> +
>>
>> Changing the disable label will also result in losing the return value of
>> pruss_cfg_read() API call here.
>>
>>>> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
>>>> + return 0;
>>>> +
>>>> + udelay(5);
>>>> + }
>>>> +
>>>> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
>>>> + ret = -ETIMEDOUT;
>>
>> Changing the disable label will also result in losing ret = -ETIMEDOUT here.
>>
>>>
>>> }
>>>
>>>> +
>>>> +disable:
>>>> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>>>> + SYSCFG_STANDBY_INIT);
>>>> + return ret;
>>>> +}
>>
>> So should I do this modification or keep it as it is?
>>
>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
>>>> +
>>>> static void pruss_of_free_clk_provider(void *data)
>>>> {
>>>> struct device_node *clk_mux_np = data;
>>>> @@ -495,10 +566,16 @@ static int pruss_remove(struct platform_device *pdev)
>>>> /* instance-specific driver private data */
>>>> static const struct pruss_private_data am437x_pruss1_data = {
>>>> .has_no_sharedram = false,
>>>> + .has_ocp_syscfg = true,
>>>> };
>>>>
>>>> static const struct pruss_private_data am437x_pruss0_data = {
>>>> .has_no_sharedram = true,
>>>> + .has_ocp_syscfg = false,
>>>> +};
>>>> +
>>>> +static const struct pruss_private_data am33xx_am57xx_data = {
>>>> + .has_ocp_syscfg = true,
>>>> };
>>>
>>> How about keeping platform data for different platforms separate?
>>>
>>> i.e. am33xx_pruss_data and am57xx_pruss_data
>>>
>>
>> Sure. I'll split am33xx_am57xx_data into am33xx_pruss_data and
>> am57xx_pruss_data as well as am65x_j721e_pruss_data into am65x_pruss_data and
>> j721e_pruss_data.
>>
>>>>
>>>> static const struct pruss_private_data am65x_j721e_pruss_data = {
>>>> @@ -506,10 +583,10 @@ static const struct pruss_private_data am65x_j721e_pruss_data = {
>>>> };
>>>>
>>>> static const struct of_device_id pruss_of_match[] = {
>>>> - { .compatible = "ti,am3356-pruss" },
>>>> + { .compatible = "ti,am3356-pruss", .data = &am33xx_am57xx_data },
>>>> { .compatible = "ti,am4376-pruss0", .data = &am437x_pruss0_data, },
>>>> { .compatible = "ti,am4376-pruss1", .data = &am437x_pruss1_data, },
>>>> - { .compatible = "ti,am5728-pruss" },
>>>> + { .compatible = "ti,am5728-pruss", .data = &am33xx_am57xx_data },
>>>> { .compatible = "ti,k2g-pruss" },
>>>> { .compatible = "ti,am654-icssg", .data = &am65x_j721e_pruss_data, },
>>>> { .compatible = "ti,j721e-icssg", .data = &am65x_j721e_pruss_data, },
>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>> index 7952f250301a..8cb99d3cad0d 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -168,6 +168,7 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>> int pruss_cfg_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
>>>> int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> unsigned int mask, unsigned int val);
>>>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable);
>>>>
>>>> #else
>>>>
>>>> @@ -203,6 +204,11 @@ static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> return -EOPNOTSUPP;
>>>> }
>>>>
>>>> +static inline int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>>>> +{
>>>> + return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> #endif /* CONFIG_TI_PRUSS */
>>>>
>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>>
>>> cheers,
>>> -roger
>>
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-08 11:29:45

[permalink] [raw]

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

On 08/03/23 16:45, Roger Quadros wrote:
>
>
> On 08/03/2023 11:23, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 08/03/23 14:04, Roger Quadros wrote:
>>> Hi Danish,
>>>
>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>> From: Suman Anna <[email protected]>
>>>>
>>>> The PRUSS CFG module is represented as a syscon node and is currently
>>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>>> pruss_cfg_update() API function.
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>> ---
>>>> include/linux/remoteproc/pruss.h | 55 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 55 insertions(+)
>>>>
>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>> index d41bec448f06..7952f250301a 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -240,4 +240,59 @@ static inline bool is_pru_rproc(struct device *dev)
>>>> return true;
>>>> }
>>>>
>>>> +/**
>>>> + * 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
>>>> + */
>>>> +static inline 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;
>>>> +
>>>
>>> Should we check for invalid gpi mode and error out if so?
>>>
>> Sure we can check for invalid gpi mode.
>>
>> Does the below code snippet looks good to you?
>>
>> if(mode < PRUSS_GPI_MODE_DIRECT || mode > PRUSS_GPI_MODE_MII)
>
> How about?
> if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>

Sure that would be better. But we will have to introduce PRUSS_GPI_MODE_MAX in
the enum definition.

Also the if() should check for mode >= PRUSS_GPI_MODE_MAX so the if check will
become,

if (mode < 0 || mode >= PRUSS_GPI_MODE_MAX)
return -EINVAL;

enum definition,

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,
};

>> 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);
>>>> +}
>>>> +
>>>> +/**
>>>> + * 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
>>>> + */
>>>> +static inline 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);
>>>> +}
>>>> +
>>>> +/**
>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>>> + * @pruss: the pruss instance
>>>> + * @enable: enable/disable
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise
>>>> + */
>>>> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable)
>>>> +{
>>>> + u32 set = enable ? PRUSS_SPP_XFER_SHIFT_EN : 0;
>>>> +
>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP,
>>>> + PRUSS_SPP_XFER_SHIFT_EN, set);
>>>> +}
>>>> +
>>>> #endif /* __LINUX_PRUSS_H */
>>>
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-08 11:38:13

[permalink] [raw]

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

Hi Roger,

On 08/03/23 13:57, Roger Quadros wrote:
> Hi,
>
> On 06/03/2023 13:09, MD Danish Anwar wrote:
>> From: Suman Anna <[email protected]>
>>
>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>> the PRUSS platform driver to allow other drivers to read and program
>> respectively a register within the PRUSS CFG sub-module represented
>> by a syscon driver. This interface provides a simple way for client
>
> Do you really need these 2 functions to be public?
> I see that later patches (4-6) add APIs for doing specific things
> and that should be sufficient than exposing entire CFG space via
> pruss_cfg_read/update().
>
>

I think the intention here is to keep this APIs pruss_cfg_read() and
pruss_cfg_update() public so that other drivers can read / modify PRUSS config
when needed.

The later patches (4-6) add APIs to do specific thing, but those APIs also
eventually call pruss_cfg_read/update().

>> drivers without having them to include and parse the CFG syscon node
>> within their respective device nodes. Various useful registers and
>> macros for certain register bit-fields and their values have also
>> been added.
>>
>> It is the responsibility of the client drivers to reconfigure or
>> reset a particular register upon any failures.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> ---
>> drivers/soc/ti/pruss.c | 41 +++++++++++++
>> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>> 2 files changed, 143 insertions(+)
>>
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index c8053c0d735f..537a3910ffd8 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>> }
>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-08 11:40:21

[permalink] [raw]

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

On 08/03/2023 13:29, Md Danish Anwar wrote:
>
>
> On 08/03/23 16:45, Roger Quadros wrote:
>>
>>
>> On 08/03/2023 11:23, Md Danish Anwar wrote:
>>> Hi Roger,
>>>
>>> On 08/03/23 14:04, Roger Quadros wrote:
>>>> Hi Danish,
>>>>
>>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>>> From: Suman Anna <[email protected]>
>>>>>
>>>>> The PRUSS CFG module is represented as a syscon node and is currently
>>>>> managed by the PRUSS platform driver. Add easy accessor functions to set
>>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable
>>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic
>>>>> pruss_cfg_update() API function.
>>>>>
>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>> ---
>>>>> include/linux/remoteproc/pruss.h | 55 ++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>>> index d41bec448f06..7952f250301a 100644
>>>>> --- a/include/linux/remoteproc/pruss.h
>>>>> +++ b/include/linux/remoteproc/pruss.h
>>>>> @@ -240,4 +240,59 @@ static inline bool is_pru_rproc(struct device *dev)
>>>>> return true;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * 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
>>>>> + */
>>>>> +static inline 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;
>>>>> +
>>>>
>>>> Should we check for invalid gpi mode and error out if so?
>>>>
>>> Sure we can check for invalid gpi mode.
>>>
>>> Does the below code snippet looks good to you?
>>>
>>> if(mode < PRUSS_GPI_MODE_DIRECT || mode > PRUSS_GPI_MODE_MII)
>>
>> How about?
>> if (mode < 0 || mode > PRUSS_GPI_MODE_MAX)
>>
>
> Sure that would be better. But we will have to introduce PRUSS_GPI_MODE_MAX in
> the enum definition.
>
> Also the if() should check for mode >= PRUSS_GPI_MODE_MAX so the if check will
> become,
>
> if (mode < 0 || mode >= PRUSS_GPI_MODE_MAX)
> return -EINVAL;
>
> enum definition,
>
> 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,
> };
>

Yes. Looks good.

>>> 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);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * 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
>>>>> + */
>>>>> +static inline 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);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality
>>>>> + * @pruss: the pruss instance
>>>>> + * @enable: enable/disable
>>>>> + *
>>>>> + * Return: 0 on success, or an error code otherwise
>>>>> + */
>>>>> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable)
>>>>> +{
>>>>> + u32 set = enable ? PRUSS_SPP_XFER_SHIFT_EN : 0;
>>>>> +
>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP,
>>>>> + PRUSS_SPP_XFER_SHIFT_EN, set);
>>>>> +}
>>>>> +
>>>>> #endif /* __LINUX_PRUSS_H */
>>>>
>>
>> cheers,
>> -roger
>

2023-03-08 11:42:38

[permalink] [raw]

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

On 08/03/2023 13:36, Md Danish Anwar wrote:
> Hi Roger,
>
> On 08/03/23 13:57, Roger Quadros wrote:
>> Hi,
>>
>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>> From: Suman Anna <[email protected]>
>>>
>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>> the PRUSS platform driver to allow other drivers to read and program
>>> respectively a register within the PRUSS CFG sub-module represented
>>> by a syscon driver. This interface provides a simple way for client
>>
>> Do you really need these 2 functions to be public?
>> I see that later patches (4-6) add APIs for doing specific things
>> and that should be sufficient than exposing entire CFG space via
>> pruss_cfg_read/update().
>>
>>
>
> I think the intention here is to keep this APIs pruss_cfg_read() and
> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
> when needed.

Where are these other drivers? If they don't exist then let's not make provision
for it now.
We can provide necessary API helpers when needed instead of letting client drivers
do what they want as they can be misused and hard to debug.

>
> The later patches (4-6) add APIs to do specific thing, but those APIs also
> eventually call pruss_cfg_read/update().

They can still call them but they need to be private to pruss.c

>
>>> drivers without having them to include and parse the CFG syscon node
>>> within their respective device nodes. Various useful registers and
>>> macros for certain register bit-fields and their values have also
>>> been added.
>>>
>>> It is the responsibility of the client drivers to reconfigure or
>>> reset a particular register upon any failures.
>>>
>>> Signed-off-by: Suman Anna <[email protected]>
>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> ---
>>> drivers/soc/ti/pruss.c | 41 +++++++++++++
>>> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>>> 2 files changed, 143 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>> index c8053c0d735f..537a3910ffd8 100644
>>> --- a/drivers/soc/ti/pruss.c
>>> +++ b/drivers/soc/ti/pruss.c
>>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>>> }
>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>
>> cheers,
>> -roger
>

2023-03-09 07:05:06

by Tony Lindgren

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

Hi,

* MD Danish Anwar <[email protected]> [230306 11:10]:
> From: Suman Anna <[email protected]>
> +/**
> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
> + * @pruss: the pruss instance handle
> + * @enable: set to true for enabling or false for disabling the OCP master ports
> + *
> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
> + * is to deassert the MStandby signal to the device PRCM, and the other is to
> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
> + * function has to wait for the PRCM to acknowledge through the monitoring of
> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
> + * disables the master access, and also signals the PRCM that the PRUSS is ready
> + * for Standby.
> + *
> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
> + * when the ready-state fails.
> + */
> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
> +{
> + int ret;
> + u32 syscfg_val, i;
> + const struct pruss_private_data *data;
> +
> + if (IS_ERR_OR_NULL(pruss))
> + return -EINVAL;
> +
> + data = of_device_get_match_data(pruss->dev);
> +
> + /* nothing to do on non OMAP-SoCs */
> + if (!data || !data->has_ocp_syscfg)
> + return 0;
> +
> + /* assert the MStandby signal during disable path */
> + if (!enable)
> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
> + SYSCFG_STANDBY_INIT,
> + SYSCFG_STANDBY_INIT);
> +
> + /* enable the OCP master ports and disable MStandby */
> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
> + if (ret)
> + return ret;
> +
> + /* wait till we are ready for transactions - delay is arbitrary */
> + for (i = 0; i < 10; i++) {
> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
> + if (ret)
> + goto disable;
> +
> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
> + return 0;
> +
> + udelay(5);
> + }
> +
> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
> + ret = -ETIMEDOUT;
> +
> +disable:
> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
> + SYSCFG_STANDBY_INIT);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);

The above you should no longer be needed, see for example am33xx-l4.dtsi
for pruss_tm: target-module@300000. The SYSCFG register is managed by
drivers/bus/ti-sysc.c using compatible "ti,sysc-pruss", and the "sysc"
reg-names property. So when any of the child devices do pm_runtime_get()
related functions, the PRUSS top level interconnect target module is
automatically enabled and disabled as needed.

If there's something still missing like dts configuration for some SoCs,
or quirk handling in ti-sysc.c, let's fix those instead so we can avoid
exporting a custom function for pruss_cfg_ocp_master_ports.

Regards,

Tony

2023-03-09 11:31:27

[permalink] [raw]

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

Hi Roger,

On 08/03/23 17:12, Roger Quadros wrote:
>
>
> On 08/03/2023 13:36, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 08/03/23 13:57, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>> From: Suman Anna <[email protected]>
>>>>
>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>> the PRUSS platform driver to allow other drivers to read and program
>>>> respectively a register within the PRUSS CFG sub-module represented
>>>> by a syscon driver. This interface provides a simple way for client
>>>
>>> Do you really need these 2 functions to be public?
>>> I see that later patches (4-6) add APIs for doing specific things
>>> and that should be sufficient than exposing entire CFG space via
>>> pruss_cfg_read/update().
>>>
>>>
>>
>> I think the intention here is to keep this APIs pruss_cfg_read() and
>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
>> when needed.
>
> Where are these other drivers? If they don't exist then let's not make provision
> for it now.
> We can provide necessary API helpers when needed instead of letting client drivers
> do what they want as they can be misused and hard to debug.
>

The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in
the series [1]. The ethernet driver series is dependent on this series. In
series [1] we are using pruss_cfg_update() in icssg_config.c file,
icssg_config() API.

So for this, the API pruss_cfg_update() needs to be public.

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

>>
>> The later patches (4-6) add APIs to do specific thing, but those APIs also
>> eventually call pruss_cfg_read/update().
>
> They can still call them but they need to be private to pruss.c
>
>>
>>>> drivers without having them to include and parse the CFG syscon node
>>>> within their respective device nodes. Various useful registers and
>>>> macros for certain register bit-fields and their values have also
>>>> been added.
>>>>
>>>> It is the responsibility of the client drivers to reconfigure or
>>>> reset a particular register upon any failures.
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>> ---
>>>> drivers/soc/ti/pruss.c | 41 +++++++++++++
>>>> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>>>> 2 files changed, 143 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index c8053c0d735f..537a3910ffd8 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>> }
>>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>
>>> cheers,
>>> -roger
>>

--
Thanks and Regards,
Danish.

2023-03-09 11:35:19

[permalink] [raw]

Subject: Re: [EXTERNAL] Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports

Hi Tony,

On 09/03/23 12:34, Tony Lindgren wrote:
> Hi,
>
> * MD Danish Anwar <[email protected]> [230306 11:10]:
>> From: Suman Anna <[email protected]>
>> +/**
>> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
>> + * @pruss: the pruss instance handle
>> + * @enable: set to true for enabling or false for disabling the OCP master ports
>> + *
>> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
>> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
>> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
>> + * is to deassert the MStandby signal to the device PRCM, and the other is to
>> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
>> + * function has to wait for the PRCM to acknowledge through the monitoring of
>> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
>> + * disables the master access, and also signals the PRCM that the PRUSS is ready
>> + * for Standby.
>> + *
>> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
>> + * when the ready-state fails.
>> + */
>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>> +{
>> + int ret;
>> + u32 syscfg_val, i;
>> + const struct pruss_private_data *data;
>> +
>> + if (IS_ERR_OR_NULL(pruss))
>> + return -EINVAL;
>> +
>> + data = of_device_get_match_data(pruss->dev);
>> +
>> + /* nothing to do on non OMAP-SoCs */
>> + if (!data || !data->has_ocp_syscfg)
>> + return 0;
>> +
>> + /* assert the MStandby signal during disable path */
>> + if (!enable)
>> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
>> + SYSCFG_STANDBY_INIT,
>> + SYSCFG_STANDBY_INIT);
>> +
>> + /* enable the OCP master ports and disable MStandby */
>> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
>> + if (ret)
>> + return ret;
>> +
>> + /* wait till we are ready for transactions - delay is arbitrary */
>> + for (i = 0; i < 10; i++) {
>> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
>> + if (ret)
>> + goto disable;
>> +
>> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
>> + return 0;
>> +
>> + udelay(5);
>> + }
>> +
>> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
>> + ret = -ETIMEDOUT;
>> +
>> +disable:
>> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>> + SYSCFG_STANDBY_INIT);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
>
> The above you should no longer be needed, see for example am33xx-l4.dtsi
> for pruss_tm: target-module@300000. The SYSCFG register is managed by
> drivers/bus/ti-sysc.c using compatible "ti,sysc-pruss", and the "sysc"
> reg-names property. So when any of the child devices do pm_runtime_get()
> related functions, the PRUSS top level interconnect target module is
> automatically enabled and disabled as needed.
>
> If there's something still missing like dts configuration for some SoCs,
> or quirk handling in ti-sysc.c, let's fix those instead so we can avoid
> exporting a custom function for pruss_cfg_ocp_master_ports.
>
> Regards,
>
> Tony

I will remove this patch in the next revision of this series as it is no longer
needed.

--
Thanks and Regards,
Danish.

2023-03-10 11:54:18

[permalink] [raw]

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

Hi Roger,

On 09/03/23 17:00, Md Danish Anwar wrote:
> Hi Roger,
>
> On 08/03/23 17:12, Roger Quadros wrote:
>>
>>
>> On 08/03/2023 13:36, Md Danish Anwar wrote:
>>> Hi Roger,
>>>
>>> On 08/03/23 13:57, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>>> From: Suman Anna <[email protected]>
>>>>>
>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>>> the PRUSS platform driver to allow other drivers to read and program
>>>>> respectively a register within the PRUSS CFG sub-module represented
>>>>> by a syscon driver. This interface provides a simple way for client
>>>>
>>>> Do you really need these 2 functions to be public?
>>>> I see that later patches (4-6) add APIs for doing specific things
>>>> and that should be sufficient than exposing entire CFG space via
>>>> pruss_cfg_read/update().
>>>>
>>>>
>>>
>>> I think the intention here is to keep this APIs pruss_cfg_read() and
>>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
>>> when needed.
>>
>> Where are these other drivers? If they don't exist then let's not make provision
>> for it now.
>> We can provide necessary API helpers when needed instead of letting client drivers
>> do what they want as they can be misused and hard to debug.
>>
>
> The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in
> the series [1]. The ethernet driver series is dependent on this series. In
> series [1] we are using pruss_cfg_update() in icssg_config.c file,
> icssg_config() API.
>
> So for this, the API pruss_cfg_update() needs to be public.
>
> [1] https://lore.kernel.org/all/[email protected]/
>

I will keep this patch as it is as pruss_cfg_update() needs to be public for
ICSSG Ethernet driver and pruss_cfg_read() is kind of a complementary function
to update. I will do required changes in other patches and send next revision
if that's OK with you. Please let me know.

>>>
>>> The later patches (4-6) add APIs to do specific thing, but those APIs also
>>> eventually call pruss_cfg_read/update().
>>
>> They can still call them but they need to be private to pruss.c
>>
>>>
>>>>> drivers without having them to include and parse the CFG syscon node
>>>>> within their respective device nodes. Various useful registers and
>>>>> macros for certain register bit-fields and their values have also
>>>>> been added.
>>>>>
>>>>> It is the responsibility of the client drivers to reconfigure or
>>>>> reset a particular register upon any failures.
>>>>>
>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>> ---
>>>>> drivers/soc/ti/pruss.c | 41 +++++++++++++
>>>>> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>>>>> 2 files changed, 143 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>> index c8053c0d735f..537a3910ffd8 100644
>>>>> --- a/drivers/soc/ti/pruss.c
>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>>
>>>> cheers,
>>>> -roger
>>>
>

--
Thanks and Regards,
Danish.

2023-03-10 13:25:41

[permalink] [raw]

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

Hi Danish,

On 10/03/2023 13:53, Md Danish Anwar wrote:
> Hi Roger,
>
> On 09/03/23 17:00, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 08/03/23 17:12, Roger Quadros wrote:
>>>
>>>
>>> On 08/03/2023 13:36, Md Danish Anwar wrote:
>>>> Hi Roger,
>>>>
>>>> On 08/03/23 13:57, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>>>> From: Suman Anna <[email protected]>
>>>>>>
>>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>>>> the PRUSS platform driver to allow other drivers to read and program
>>>>>> respectively a register within the PRUSS CFG sub-module represented
>>>>>> by a syscon driver. This interface provides a simple way for client
>>>>>
>>>>> Do you really need these 2 functions to be public?
>>>>> I see that later patches (4-6) add APIs for doing specific things
>>>>> and that should be sufficient than exposing entire CFG space via
>>>>> pruss_cfg_read/update().
>>>>>
>>>>>
>>>>
>>>> I think the intention here is to keep this APIs pruss_cfg_read() and
>>>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
>>>> when needed.
>>>
>>> Where are these other drivers? If they don't exist then let's not make provision
>>> for it now.
>>> We can provide necessary API helpers when needed instead of letting client drivers
>>> do what they want as they can be misused and hard to debug.
>>>
>>
>> The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in
>> the series [1]. The ethernet driver series is dependent on this series. In
>> series [1] we are using pruss_cfg_update() in icssg_config.c file,
>> icssg_config() API.

You can instead add a new API on what exactly you want it to do rather than exposing
entire CFG space.

>>
>> So for this, the API pruss_cfg_update() needs to be public.
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>>
>
> I will keep this patch as it is as pruss_cfg_update() needs to be public for
> ICSSG Ethernet driver and pruss_cfg_read() is kind of a complementary function
> to update. I will do required changes in other patches and send next revision
> if that's OK with you. Please let me know.
>
>>>>
>>>> The later patches (4-6) add APIs to do specific thing, but those APIs also
>>>> eventually call pruss_cfg_read/update().
>>>
>>> They can still call them but they need to be private to pruss.c
>>>
>>>>
>>>>>> drivers without having them to include and parse the CFG syscon node
>>>>>> within their respective device nodes. Various useful registers and
>>>>>> macros for certain register bit-fields and their values have also
>>>>>> been added.
>>>>>>
>>>>>> It is the responsibility of the client drivers to reconfigure or
>>>>>> reset a particular register upon any failures.
>>>>>>
>>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>>> ---
>>>>>> drivers/soc/ti/pruss.c | 41 +++++++++++++
>>>>>> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 143 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>>> index c8053c0d735f..537a3910ffd8 100644
>>>>>> --- a/drivers/soc/ti/pruss.c
>>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>>>

cheers,
-roger

2023-03-10 15:47:15

[permalink] [raw]

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

Hi Roger,

On 10/03/23 18:53, Roger Quadros wrote:
> Hi Danish,
>
> On 10/03/2023 13:53, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 09/03/23 17:00, Md Danish Anwar wrote:
>>> Hi Roger,
>>>
>>> On 08/03/23 17:12, Roger Quadros wrote:
>>>>
>>>>
>>>> On 08/03/2023 13:36, Md Danish Anwar wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 08/03/23 13:57, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>>>>> From: Suman Anna <[email protected]>
>>>>>>>
>>>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>>>>> the PRUSS platform driver to allow other drivers to read and program
>>>>>>> respectively a register within the PRUSS CFG sub-module represented
>>>>>>> by a syscon driver. This interface provides a simple way for client
>>>>>>
>>>>>> Do you really need these 2 functions to be public?
>>>>>> I see that later patches (4-6) add APIs for doing specific things
>>>>>> and that should be sufficient than exposing entire CFG space via
>>>>>> pruss_cfg_read/update().
>>>>>>
>>>>>>
>>>>>
>>>>> I think the intention here is to keep this APIs pruss_cfg_read() and
>>>>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
>>>>> when needed.
>>>>
>>>> Where are these other drivers? If they don't exist then let's not make provision
>>>> for it now.
>>>> We can provide necessary API helpers when needed instead of letting client drivers
>>>> do what they want as they can be misused and hard to debug.
>>>>
>>>
>>> The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in
>>> the series [1]. The ethernet driver series is dependent on this series. In
>>> series [1] we are using pruss_cfg_update() in icssg_config.c file,
>>> icssg_config() API.
>
> You can instead add a new API on what exactly you want it to do rather than exposing
> entire CFG space.
>

Sure.

In icssg_config.c, a call to pruss_cfg_update() is made to enable XFR shift for
PRU and RTU,

/* enable XFR shift for PRU and RTU */
mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
pruss_cfg_update(prueth->pruss, PRUSS_CFG_SPP, mask, mask);

I will add the below API as part of Patch 4 of the series. We'll call this API
and entire CFG space will not be exposed.

/**
* pruss_cfg_xfr_pru_rtu_enable() - Enable/disable XFR shift for PRU and RTU
* @pruss: the pruss instance
* @enable: enable/disable
*
* Return: 0 on success, or an error code otherwise
*/
static inline int pruss_cfg_xfr_pru_rtu_enable(struct pruss *pruss, bool enable)
{
u32 mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
u32 set = enable ? mask : 0;

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

To make pruss_cfg_update() and pruss_cfg_read() API internal to pruss.c, I will
add the below change to pruss.h file and pruss.c file. Let me know if this
change looks okay to you.

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c

index 537a3910ffd8..9f01c8809deb 100644

--- a/drivers/soc/ti/pruss.c

+++ b/drivers/soc/ti/pruss.c

@@ -182,7 +182,6 @@ int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
unsigned int *val)

return regmap_read(pruss->cfg_regmap, reg, val);

}

-EXPORT_SYMBOL_GPL(pruss_cfg_read);

/**

* pruss_cfg_update() - configure a PRUSS CFG sub-module register

@@ -203,7 +202,6 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,

return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);

}

-EXPORT_SYMBOL_GPL(pruss_cfg_update);

static void pruss_of_free_clk_provider(void *data)

{

diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h

index d41bec448f06..12ef10b9fe9a 100644

--- a/include/linux/remoteproc/pruss.h

+++ b/include/linux/remoteproc/pruss.h

@@ -165,9 +165,6 @@ 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_read(struct pruss *pruss, unsigned int reg, unsigned int *val);

-int pruss_cfg_update(struct pruss *pruss, unsigned int reg,

- unsigned int mask, unsigned int val);

#else

@@ -191,18 +188,6 @@ static inline int pruss_release_mem_region(struct pruss
*pruss,

return -EOPNOTSUPP;

}

-static inline int pruss_cfg_read(struct pruss *pruss, unsigned int reg,

- unsigned int *val)

-{

- return -EOPNOTSUPP;

-}

-

-static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,

- unsigned int mask, unsigned int val)

-{

- return -EOPNOTSUPP;

-}

-

#endif /* CONFIG_TI_PRUSS */

#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)

Please have a look and let me know if above API and code changes looks OK to you.

>>>
>>> So for this, the API pruss_cfg_update() needs to be public.
>>>
>>> [1] https://lore.kernel.org/all/[email protected]/
>>>
>>
>> I will keep this patch as it is as pruss_cfg_update() needs to be public for
>> ICSSG Ethernet driver and pruss_cfg_read() is kind of a complementary function
>> to update. I will do required changes in other patches and send next revision
>> if that's OK with you. Please let me know.
>>
>>>>>
>>>>> The later patches (4-6) add APIs to do specific thing, but those APIs also
>>>>> eventually call pruss_cfg_read/update().
>>>>
>>>> They can still call them but they need to be private to pruss.c
>>>>
>>>>>
>>>>>>> drivers without having them to include and parse the CFG syscon node
>>>>>>> within their respective device nodes. Various useful registers and
>>>>>>> macros for certain register bit-fields and their values have also
>>>>>>> been added.
>>>>>>>
>>>>>>> It is the responsibility of the client drivers to reconfigure or
>>>>>>> reset a particular register upon any failures.
>>>>>>>
>>>>>>> Signed-off-by: Suman Anna <[email protected]>
>>>>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>>>>> ---
>>>>>>> drivers/soc/ti/pruss.c | 41 +++++++++++++
>>>>>>> include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
>>>>>>> 2 files changed, 143 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>>>>> index c8053c0d735f..537a3910ffd8 100644
>>>>>>> --- a/drivers/soc/ti/pruss.c
>>>>>>> +++ b/drivers/soc/ti/pruss.c
>>>>>>> @@ -164,6 +164,47 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>>>>> }
>>>>>>> EXPORT_SYMBOL_GPL(pruss_release_mem_region);
>>>>>>
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-11 12:07:08

[permalink] [raw]

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

Hi Danish,

On 10/03/2023 17:36, Md Danish Anwar wrote:
> Hi Roger,
>
> On 10/03/23 18:53, Roger Quadros wrote:
>> Hi Danish,
>>
>> On 10/03/2023 13:53, Md Danish Anwar wrote:
>>> Hi Roger,
>>>
>>> On 09/03/23 17:00, Md Danish Anwar wrote:
>>>> Hi Roger,
>>>>
>>>> On 08/03/23 17:12, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 08/03/2023 13:36, Md Danish Anwar wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On 08/03/23 13:57, Roger Quadros wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>>>>>> From: Suman Anna <[email protected]>
>>>>>>>>
>>>>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>>>>>> the PRUSS platform driver to allow other drivers to read and program
>>>>>>>> respectively a register within the PRUSS CFG sub-module represented
>>>>>>>> by a syscon driver. This interface provides a simple way for client
>>>>>>>
>>>>>>> Do you really need these 2 functions to be public?
>>>>>>> I see that later patches (4-6) add APIs for doing specific things
>>>>>>> and that should be sufficient than exposing entire CFG space via
>>>>>>> pruss_cfg_read/update().
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> I think the intention here is to keep this APIs pruss_cfg_read() and
>>>>>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
>>>>>> when needed.
>>>>>
>>>>> Where are these other drivers? If they don't exist then let's not make provision
>>>>> for it now.
>>>>> We can provide necessary API helpers when needed instead of letting client drivers
>>>>> do what they want as they can be misused and hard to debug.
>>>>>
>>>>
>>>> The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in
>>>> the series [1]. The ethernet driver series is dependent on this series. In
>>>> series [1] we are using pruss_cfg_update() in icssg_config.c file,
>>>> icssg_config() API.
>>
>> You can instead add a new API on what exactly you want it to do rather than exposing
>> entire CFG space.
>>
>
> Sure.
>
> In icssg_config.c, a call to pruss_cfg_update() is made to enable XFR shift for
> PRU and RTU,
>
> /* enable XFR shift for PRU and RTU */
> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
> pruss_cfg_update(prueth->pruss, PRUSS_CFG_SPP, mask, mask);
>
> I will add the below API as part of Patch 4 of the series. We'll call this API
> and entire CFG space will not be exposed.
>
> /**
> * pruss_cfg_xfr_pru_rtu_enable() - Enable/disable XFR shift for PRU and RTU
> * @pruss: the pruss instance
> * @enable: enable/disable
> *
> * Return: 0 on success, or an error code otherwise
> */
> static inline int pruss_cfg_xfr_pru_rtu_enable(struct pruss *pruss, bool enable)
> {
> u32 mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
> u32 set = enable ? mask : 0;
>
> return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
> }

I would suggest to make separate APIs for PRU XFR vs RTU XFR.

>
> To make pruss_cfg_update() and pruss_cfg_read() API internal to pruss.c, I will
> add the below change to pruss.h file and pruss.c file. Let me know if this
> change looks okay to you.
>
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>
> index 537a3910ffd8..9f01c8809deb 100644
>
> --- a/drivers/soc/ti/pruss.c
>
> +++ b/drivers/soc/ti/pruss.c
>
> @@ -182,7 +182,6 @@ int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
> unsigned int *val)

Need to declare this as 'static'.

>
>
>
> return regmap_read(pruss->cfg_regmap, reg, val);
>
> }
>
> -EXPORT_SYMBOL_GPL(pruss_cfg_read);
>
>
>
> /**
>
> * pruss_cfg_update() - configure a PRUSS CFG sub-module register
>
> @@ -203,7 +202,6 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,

this as well.

>
>
>
> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>
> }
>
> -EXPORT_SYMBOL_GPL(pruss_cfg_update);
>
>
>
> static void pruss_of_free_clk_provider(void *data)
>
> {
>
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>
> index d41bec448f06..12ef10b9fe9a 100644
>
> --- a/include/linux/remoteproc/pruss.h
>
> +++ b/include/linux/remoteproc/pruss.h
>
> @@ -165,9 +165,6 @@ 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_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
>
> -int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>
> - unsigned int mask, unsigned int val);
>
>
>
> #else
>
>
>
> @@ -191,18 +188,6 @@ static inline int pruss_release_mem_region(struct pruss
> *pruss,
>
> return -EOPNOTSUPP;
>
> }
>
>
>
> -static inline int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
>
> - unsigned int *val)
>
> -{
>
> - return -EOPNOTSUPP;
>
> -}
>
> -
>
> -static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>
> - unsigned int mask, unsigned int val)
>
> -{
>
> - return -EOPNOTSUPP;
>
> -}
>
> -
>
> #endif /* CONFIG_TI_PRUSS */
>
>
>
> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>
>
> Please have a look and let me know if above API and code changes looks OK to you.
>

Rest looks OK.

cheers,
-roger

2023-03-13 05:01:41

[permalink] [raw]

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

Hi Roger

On 11/03/23 17:36, Roger Quadros wrote:
> Hi Danish,
>
> On 10/03/2023 17:36, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 10/03/23 18:53, Roger Quadros wrote:
>>> Hi Danish,
>>>
>>> On 10/03/2023 13:53, Md Danish Anwar wrote:
>>>> Hi Roger,
>>>>
>>>> On 09/03/23 17:00, Md Danish Anwar wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 08/03/23 17:12, Roger Quadros wrote:
>>>>>>
>>>>>>
>>>>>> On 08/03/2023 13:36, Md Danish Anwar wrote:
>>>>>>> Hi Roger,
>>>>>>>
>>>>>>> On 08/03/23 13:57, Roger Quadros wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>>>>>>> From: Suman Anna <[email protected]>
>>>>>>>>>
>>>>>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>>>>>>>>> the PRUSS platform driver to allow other drivers to read and program
>>>>>>>>> respectively a register within the PRUSS CFG sub-module represented
>>>>>>>>> by a syscon driver. This interface provides a simple way for client
>>>>>>>>
>>>>>>>> Do you really need these 2 functions to be public?
>>>>>>>> I see that later patches (4-6) add APIs for doing specific things
>>>>>>>> and that should be sufficient than exposing entire CFG space via
>>>>>>>> pruss_cfg_read/update().
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> I think the intention here is to keep this APIs pruss_cfg_read() and
>>>>>>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config
>>>>>>> when needed.
>>>>>>
>>>>>> Where are these other drivers? If they don't exist then let's not make provision
>>>>>> for it now.
>>>>>> We can provide necessary API helpers when needed instead of letting client drivers
>>>>>> do what they want as they can be misused and hard to debug.
>>>>>>
>>>>>
>>>>> The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in
>>>>> the series [1]. The ethernet driver series is dependent on this series. In
>>>>> series [1] we are using pruss_cfg_update() in icssg_config.c file,
>>>>> icssg_config() API.
>>>
>>> You can instead add a new API on what exactly you want it to do rather than exposing
>>> entire CFG space.
>>>
>>
>> Sure.
>>
>> In icssg_config.c, a call to pruss_cfg_update() is made to enable XFR shift for
>> PRU and RTU,
>>
>> /* enable XFR shift for PRU and RTU */
>> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
>> pruss_cfg_update(prueth->pruss, PRUSS_CFG_SPP, mask, mask);
>>
>> I will add the below API as part of Patch 4 of the series. We'll call this API
>> and entire CFG space will not be exposed.
>>
>> /**
>> * pruss_cfg_xfr_pru_rtu_enable() - Enable/disable XFR shift for PRU and RTU
>> * @pruss: the pruss instance
>> * @enable: enable/disable
>> *
>> * Return: 0 on success, or an error code otherwise
>> */
>> static inline int pruss_cfg_xfr_pru_rtu_enable(struct pruss *pruss, bool enable)
>> {
>> u32 mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN;
>> u32 set = enable ? mask : 0;
>>
>> return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set);
>> }
>
> I would suggest to make separate APIs for PRU XFR vs RTU XFR.
>

How about making only one API for XFR shift and passing PRU or RTU as argument
to the API. The API along with struct pruss and bool enable will take another
argument u32 mask.

mask = PRUSS_SPP_XFER_SHIFT_EN for PRU
mask = PRUSS_SPP_RTU_XFR_SHIFT_EN for RTU
mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN for PRU and RTU

So one API will be able to do all three jobs.

How does this seem?

>>
>> To make pruss_cfg_update() and pruss_cfg_read() API internal to pruss.c, I will
>> add the below change to pruss.h file and pruss.c file. Let me know if this
>> change looks okay to you.
>>
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>
>> index 537a3910ffd8..9f01c8809deb 100644
>>
>> --- a/drivers/soc/ti/pruss.c
>>
>> +++ b/drivers/soc/ti/pruss.c
>>
>> @@ -182,7 +182,6 @@ int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
>> unsigned int *val)
>
> Need to declare this as 'static'.
>
>>
>>
>>
>> return regmap_read(pruss->cfg_regmap, reg, val);
>>
>> }
>>
>> -EXPORT_SYMBOL_GPL(pruss_cfg_read);
>>
>>
>>
>> /**
>>
>> * pruss_cfg_update() - configure a PRUSS CFG sub-module register
>>
>> @@ -203,7 +202,6 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>
> this as well.
>
>>
>>
>>
>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val);
>>
>> }
>>
>> -EXPORT_SYMBOL_GPL(pruss_cfg_update);
>>
>>
>>
>> static void pruss_of_free_clk_provider(void *data)
>>
>> {
>>
>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>
>> index d41bec448f06..12ef10b9fe9a 100644
>>
>> --- a/include/linux/remoteproc/pruss.h
>>
>> +++ b/include/linux/remoteproc/pruss.h
>>
>> @@ -165,9 +165,6 @@ 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_read(struct pruss *pruss, unsigned int reg, unsigned int *val);
>>
>> -int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>
>> - unsigned int mask, unsigned int val);
>>
>>
>>
>> #else
>>
>>
>>
>> @@ -191,18 +188,6 @@ static inline int pruss_release_mem_region(struct pruss
>> *pruss,
>>
>> return -EOPNOTSUPP;
>>
>> }
>>
>>
>>
>> -static inline int pruss_cfg_read(struct pruss *pruss, unsigned int reg,
>>
>> - unsigned int *val)
>>
>> -{
>>
>> - return -EOPNOTSUPP;
>>
>> -}
>>
>> -
>>
>> -static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>
>> - unsigned int mask, unsigned int val)
>>
>> -{
>>
>> - return -EOPNOTSUPP;
>>
>> -}
>>
>> -
>>
>> #endif /* CONFIG_TI_PRUSS */
>>
>>
>>
>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>
>>
>> Please have a look and let me know if above API and code changes looks OK to you.
>>
>
> Rest looks OK.
>
> cheers,
> -roger

--
Thanks and Regards,
Danish.

2023-03-13 07:51:26