From: Tero Kristo <[email protected]>
Add two new get and put API, pruss_get() and pruss_put() to the
PRUSS platform driver to allow client drivers to request a handle
to a PRUSS device. This handle will be used by client drivers to
request various operations of the PRUSS platform driver through
additional API that will be added in the following patches.
The pruss_get() function returns the pruss handle corresponding
to a PRUSS device referenced by a PRU remoteproc instance. The
pruss_put() is the complimentary function to pruss_get().
Co-developed-by: Suman Anna <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
Signed-off-by: Tero Kristo <[email protected]>
Co-developed-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Grzegorz Jaszczyk <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
Signed-off-by: MD Danish Anwar <[email protected]>
Reviewed-by: Roger Quadros <[email protected]>
---
drivers/remoteproc/pru_rproc.c | 2 +-
drivers/soc/ti/pruss.c | 60 ++++++++++++++++++-
.../{pruss_driver.h => pruss_internal.h} | 7 ++-
include/linux/remoteproc/pruss.h | 19 ++++++
4 files changed, 83 insertions(+), 5 deletions(-)
rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index b76db7fa693d..4ddd5854d56e 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -19,7 +19,7 @@
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/remoteproc/pruss.h>
-#include <linux/pruss_driver.h>
+#include <linux/pruss_internal.h>
#include <linux/remoteproc.h>
#include "remoteproc_internal.h"
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 6882c86b3ce5..6c2bb02a521d 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -6,6 +6,7 @@
* Author(s):
* Suman Anna <[email protected]>
* Andrew F. Davis <[email protected]>
+ * Tero Kristo <[email protected]>
*/
#include <linux/clk-provider.h>
@@ -16,8 +17,9 @@
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/pm_runtime.h>
-#include <linux/pruss_driver.h>
+#include <linux/pruss_internal.h>
#include <linux/regmap.h>
+#include <linux/remoteproc.h>
#include <linux/slab.h>
/**
@@ -30,6 +32,62 @@ struct pruss_private_data {
bool has_core_mux_clock;
};
+/**
+ * pruss_get() - get the pruss for a given PRU remoteproc
+ * @rproc: remoteproc handle of a PRU instance
+ *
+ * Finds the parent pruss device for a PRU given the @rproc handle of the
+ * PRU remote processor. This function increments the pruss device's refcount,
+ * so always use pruss_put() to decrement it back once pruss isn't needed
+ * anymore.
+ *
+ * Return: pruss handle on success, and an ERR_PTR on failure using one
+ * of the following error values
+ * -EINVAL if invalid parameter
+ * -ENODEV if PRU device or PRUSS device is not found
+ */
+struct pruss *pruss_get(struct rproc *rproc)
+{
+ struct pruss *pruss;
+ struct device *dev;
+ struct platform_device *ppdev;
+
+ if (IS_ERR_OR_NULL(rproc))
+ return ERR_PTR(-EINVAL);
+
+ dev = &rproc->dev;
+
+ /* make sure it is PRU rproc */
+ if (!dev->parent || !is_pru_rproc(dev->parent))
+ return ERR_PTR(-ENODEV);
+
+ ppdev = to_platform_device(dev->parent->parent);
+ pruss = platform_get_drvdata(ppdev);
+ if (!pruss)
+ return ERR_PTR(-ENODEV);
+
+ get_device(pruss->dev);
+
+ return pruss;
+}
+EXPORT_SYMBOL_GPL(pruss_get);
+
+/**
+ * pruss_put() - decrement pruss device's usecount
+ * @pruss: pruss handle
+ *
+ * Complimentary function for pruss_get(). Needs to be called
+ * after the PRUSS is used, and only if the pruss_get() succeeds.
+ */
+void pruss_put(struct pruss *pruss)
+{
+ if (IS_ERR_OR_NULL(pruss))
+ return;
+
+ put_device(pruss->dev);
+}
+EXPORT_SYMBOL_GPL(pruss_put);
+
static void pruss_of_free_clk_provider(void *data)
{
struct device_node *clk_mux_np = data;
diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
similarity index 90%
rename from include/linux/pruss_driver.h
rename to include/linux/pruss_internal.h
index ecfded30ed05..8f91cb164054 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_internal.h
@@ -6,9 +6,10 @@
* Suman Anna <[email protected]>
*/
-#ifndef _PRUSS_DRIVER_H_
-#define _PRUSS_DRIVER_H_
+#ifndef _PRUSS_INTERNAL_H_
+#define _PRUSS_INTERNAL_H_
+#include <linux/remoteproc/pruss.h>
#include <linux/types.h>
/*
@@ -51,4 +52,4 @@ struct pruss {
struct clk *iep_clk_mux;
};
-#endif /* _PRUSS_DRIVER_H_ */
+#endif /* _PRUSS_INTERNAL_H_ */
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 039b50d58df2..93a98cac7829 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -4,12 +4,14 @@
*
* Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
* Suman Anna <[email protected]>
+ * Tero Kristo <[email protected]>
*/
#ifndef __LINUX_PRUSS_H
#define __LINUX_PRUSS_H
#include <linux/device.h>
+#include <linux/err.h>
#include <linux/types.h>
#define PRU_RPROC_DRVNAME "pru-rproc"
@@ -44,6 +46,23 @@ enum pru_ctable_idx {
struct device_node;
struct rproc;
+struct pruss;
+
+#if IS_ENABLED(CONFIG_TI_PRUSS)
+
+struct pruss *pruss_get(struct rproc *rproc);
+void pruss_put(struct pruss *pruss);
+
+#else
+
+static inline struct pruss *pruss_get(struct rproc *rproc)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void pruss_put(struct pruss *pruss) { }
+
+#endif /* CONFIG_TI_PRUSS */
#if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
--
2.25.1
Hi Danish
On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
> From: Tero Kristo <[email protected]>
>
> Add two new get and put API, pruss_get() and pruss_put() to the
> PRUSS platform driver to allow client drivers to request a handle
> to a PRUSS device. This handle will be used by client drivers to
> request various operations of the PRUSS platform driver through
> additional API that will be added in the following patches.
>
> The pruss_get() function returns the pruss handle corresponding
> to a PRUSS device referenced by a PRU remoteproc instance. The
> pruss_put() is the complimentary function to pruss_get().
>
> Co-developed-by: Suman Anna <[email protected]>
> Signed-off-by: Suman Anna <[email protected]>
> Signed-off-by: Tero Kristo <[email protected]>
> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> Signed-off-by: Puranjay Mohan <[email protected]>
> Signed-off-by: MD Danish Anwar <[email protected]>
> Reviewed-by: Roger Quadros <[email protected]>
> ---
> drivers/remoteproc/pru_rproc.c | 2 +-
> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++-
> .../{pruss_driver.h => pruss_internal.h} | 7 ++-
> include/linux/remoteproc/pruss.h | 19 ++++++
> 4 files changed, 83 insertions(+), 5 deletions(-)
> rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index b76db7fa693d..4ddd5854d56e 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -19,7 +19,7 @@
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> #include <linux/remoteproc/pruss.h>
> -#include <linux/pruss_driver.h>
> +#include <linux/pruss_internal.h>
> #include <linux/remoteproc.h>
>
> #include "remoteproc_internal.h"
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 6882c86b3ce5..6c2bb02a521d 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -6,6 +6,7 @@
> * Author(s):
> * Suman Anna <[email protected]>
> * Andrew F. Davis <[email protected]>
> + * Tero Kristo <[email protected]>
> */
>
> #include <linux/clk-provider.h>
> @@ -16,8 +17,9 @@
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> #include <linux/pm_runtime.h>
> -#include <linux/pruss_driver.h>
> +#include <linux/pruss_internal.h>
> #include <linux/regmap.h>
> +#include <linux/remoteproc.h>
> #include <linux/slab.h>
>
> /**
> @@ -30,6 +32,62 @@ struct pruss_private_data {
> bool has_core_mux_clock;
> };
>
> +/**
> + * pruss_get() - get the pruss for a given PRU remoteproc
> + * @rproc: remoteproc handle of a PRU instance
> + *
> + * Finds the parent pruss device for a PRU given the @rproc handle of the
> + * PRU remote processor. This function increments the pruss device's refcount,
> + * so always use pruss_put() to decrement it back once pruss isn't needed
> + * anymore.
> + *
> + * Return: pruss handle on success, and an ERR_PTR on failure using one
> + * of the following error values
> + * -EINVAL if invalid parameter
> + * -ENODEV if PRU device or PRUSS device is not found
> + */
> +struct pruss *pruss_get(struct rproc *rproc)
> +{
> + struct pruss *pruss;
> + struct device *dev;
> + struct platform_device *ppdev;
> +
> + if (IS_ERR_OR_NULL(rproc))
> + return ERR_PTR(-EINVAL);
> +
There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
or pru_rproc_get().
> + dev = &rproc->dev;
> +
> + /* make sure it is PRU rproc */
> + if (!dev->parent || !is_pru_rproc(dev->parent))
> + return ERR_PTR(-ENODEV);
> +
> + ppdev = to_platform_device(dev->parent->parent);
> + pruss = platform_get_drvdata(ppdev);
> + if (!pruss)
> + return ERR_PTR(-ENODEV);
> +
> + get_device(pruss->dev);
> +
> + return pruss;
> +}
> +EXPORT_SYMBOL_GPL(pruss_get);
> +
> +/**
> + * pruss_put() - decrement pruss device's usecount
> + * @pruss: pruss handle
> + *
> + * Complimentary function for pruss_get(). Needs to be called
> + * after the PRUSS is used, and only if the pruss_get() succeeds.
> + */
> +void pruss_put(struct pruss *pruss)
> +{
> + if (IS_ERR_OR_NULL(pruss))
> + return;
> +
> + put_device(pruss->dev);
> +}
> +EXPORT_SYMBOL_GPL(pruss_put);
> +
> static void pruss_of_free_clk_provider(void *data)
> {
> struct device_node *clk_mux_np = data;
> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
> similarity index 90%
> rename from include/linux/pruss_driver.h
> rename to include/linux/pruss_internal.h
> index ecfded30ed05..8f91cb164054 100644
> --- a/include/linux/pruss_driver.h
> +++ b/include/linux/pruss_internal.h
> @@ -6,9 +6,10 @@
> * Suman Anna <[email protected]>
> */
>
> -#ifndef _PRUSS_DRIVER_H_
> -#define _PRUSS_DRIVER_H_
> +#ifndef _PRUSS_INTERNAL_H_
> +#define _PRUSS_INTERNAL_H_
>
> +#include <linux/remoteproc/pruss.h>
> #include <linux/types.h>
>
> /*
> @@ -51,4 +52,4 @@ struct pruss {
> struct clk *iep_clk_mux;
> };
>
> -#endif /* _PRUSS_DRIVER_H_ */
> +#endif /* _PRUSS_INTERNAL_H_ */
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> index 039b50d58df2..93a98cac7829 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -4,12 +4,14 @@
> *
> * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
> * Suman Anna <[email protected]>
> + * Tero Kristo <[email protected]>
> */
>
> #ifndef __LINUX_PRUSS_H
> #define __LINUX_PRUSS_H
>
> #include <linux/device.h>
> +#include <linux/err.h>
> #include <linux/types.h>
>
> #define PRU_RPROC_DRVNAME "pru-rproc"
> @@ -44,6 +46,23 @@ enum pru_ctable_idx {
>
> struct device_node;
> struct rproc;
> +struct pruss;
> +
> +#if IS_ENABLED(CONFIG_TI_PRUSS)
> +
> +struct pruss *pruss_get(struct rproc *rproc);
> +void pruss_put(struct pruss *pruss);
> +
> +#else
> +
> +static inline struct pruss *pruss_get(struct rproc *rproc)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void pruss_put(struct pruss *pruss) { }
> +
> +#endif /* CONFIG_TI_PRUSS */
>
> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>
> --
> 2.25.1
>
Hi Mathieu,
On 28/03/23 02:28, Mathieu Poirier wrote:
> Hi Danish
>
> On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
>> From: Tero Kristo <[email protected]>
>>
>> Add two new get and put API, pruss_get() and pruss_put() to the
>> PRUSS platform driver to allow client drivers to request a handle
>> to a PRUSS device. This handle will be used by client drivers to
>> request various operations of the PRUSS platform driver through
>> additional API that will be added in the following patches.
>>
>> The pruss_get() function returns the pruss handle corresponding
>> to a PRUSS device referenced by a PRU remoteproc instance. The
>> pruss_put() is the complimentary function to pruss_get().
>>
>> Co-developed-by: Suman Anna <[email protected]>
>> Signed-off-by: Suman Anna <[email protected]>
>> Signed-off-by: Tero Kristo <[email protected]>
>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> Signed-off-by: MD Danish Anwar <[email protected]>
>> Reviewed-by: Roger Quadros <[email protected]>
>> ---
>> drivers/remoteproc/pru_rproc.c | 2 +-
>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++-
>> .../{pruss_driver.h => pruss_internal.h} | 7 ++-
>> include/linux/remoteproc/pruss.h | 19 ++++++
>> 4 files changed, 83 insertions(+), 5 deletions(-)
>> rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>>
>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>> index b76db7fa693d..4ddd5854d56e 100644
>> --- a/drivers/remoteproc/pru_rproc.c
>> +++ b/drivers/remoteproc/pru_rproc.c
>> @@ -19,7 +19,7 @@
>> #include <linux/of_device.h>
>> #include <linux/of_irq.h>
>> #include <linux/remoteproc/pruss.h>
>> -#include <linux/pruss_driver.h>
>> +#include <linux/pruss_internal.h>
>> #include <linux/remoteproc.h>
>>
>> #include "remoteproc_internal.h"
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 6882c86b3ce5..6c2bb02a521d 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -6,6 +6,7 @@
>> * Author(s):
>> * Suman Anna <[email protected]>
>> * Andrew F. Davis <[email protected]>
>> + * Tero Kristo <[email protected]>
>> */
>>
>> #include <linux/clk-provider.h>
>> @@ -16,8 +17,9 @@
>> #include <linux/of_address.h>
>> #include <linux/of_device.h>
>> #include <linux/pm_runtime.h>
>> -#include <linux/pruss_driver.h>
>> +#include <linux/pruss_internal.h>
>> #include <linux/regmap.h>
>> +#include <linux/remoteproc.h>
>> #include <linux/slab.h>
>>
>> /**
>> @@ -30,6 +32,62 @@ struct pruss_private_data {
>> bool has_core_mux_clock;
>> };
>>
>> +/**
>> + * pruss_get() - get the pruss for a given PRU remoteproc
>> + * @rproc: remoteproc handle of a PRU instance
>> + *
>> + * Finds the parent pruss device for a PRU given the @rproc handle of the
>> + * PRU remote processor. This function increments the pruss device's refcount,
>> + * so always use pruss_put() to decrement it back once pruss isn't needed
>> + * anymore.
>> + *
>> + * Return: pruss handle on success, and an ERR_PTR on failure using one
>> + * of the following error values
>> + * -EINVAL if invalid parameter
>> + * -ENODEV if PRU device or PRUSS device is not found
>> + */
>> +struct pruss *pruss_get(struct rproc *rproc)
>> +{
>> + struct pruss *pruss;
>> + struct device *dev;
>> + struct platform_device *ppdev;
>> +
>> + if (IS_ERR_OR_NULL(rproc))
>> + return ERR_PTR(-EINVAL);
>> +
>
> There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
> or pru_rproc_get().
>
Here in this API, we are checking if rproc is NULL or not. Also we are checking
is_pru_rproc() to make sure this rproc is pru-rproc only and not some other rproc.
This API will be called from driver (icssg_prueth.c) which I'll post once this
series is merged.
In the driver we are doing,
prueth->pru[slice] = pru_rproc_get(np, pru, &pruss_id);
pruss = pruss_get(prueth->pru[slice]);
So, before calling pruss_get() we are in fact calling pru_rproc_get() to make
sure it's a valid rproc.
I think in this API, these two checks (NULL check and is_pru_rproc) should be
OK as the driver is already calling pru_rproc_get() before this API.
The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
is_pru_rproc() will only be true if it is a "pru-rproc" implying
pru_rproc_get() was called before calling this API.
Please let me know if this is OK or if any change is required.
>> + dev = &rproc->dev;
>> +
>> + /* make sure it is PRU rproc */
>> + if (!dev->parent || !is_pru_rproc(dev->parent))
>> + return ERR_PTR(-ENODEV);
>> +
>> + ppdev = to_platform_device(dev->parent->parent);
>> + pruss = platform_get_drvdata(ppdev);
>> + if (!pruss)
>> + return ERR_PTR(-ENODEV);
>> +
>> + get_device(pruss->dev);
>> +
>> + return pruss;
>> +}
>> +EXPORT_SYMBOL_GPL(pruss_get);
>> +
>> +/**
>> + * pruss_put() - decrement pruss device's usecount
>> + * @pruss: pruss handle
>> + *
>> + * Complimentary function for pruss_get(). Needs to be called
>> + * after the PRUSS is used, and only if the pruss_get() succeeds.
>> + */
>> +void pruss_put(struct pruss *pruss)
>> +{
>> + if (IS_ERR_OR_NULL(pruss))
>> + return;
>> +
>> + put_device(pruss->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(pruss_put);
>> +
>> static void pruss_of_free_clk_provider(void *data)
>> {
>> struct device_node *clk_mux_np = data;
>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
>> similarity index 90%
>> rename from include/linux/pruss_driver.h
>> rename to include/linux/pruss_internal.h
>> index ecfded30ed05..8f91cb164054 100644
>> --- a/include/linux/pruss_driver.h
>> +++ b/include/linux/pruss_internal.h
>> @@ -6,9 +6,10 @@
>> * Suman Anna <[email protected]>
>> */
>>
>> -#ifndef _PRUSS_DRIVER_H_
>> -#define _PRUSS_DRIVER_H_
>> +#ifndef _PRUSS_INTERNAL_H_
>> +#define _PRUSS_INTERNAL_H_
>>
>> +#include <linux/remoteproc/pruss.h>
>> #include <linux/types.h>
>>
>> /*
>> @@ -51,4 +52,4 @@ struct pruss {
>> struct clk *iep_clk_mux;
>> };
>>
>> -#endif /* _PRUSS_DRIVER_H_ */
>> +#endif /* _PRUSS_INTERNAL_H_ */
>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>> index 039b50d58df2..93a98cac7829 100644
>> --- a/include/linux/remoteproc/pruss.h
>> +++ b/include/linux/remoteproc/pruss.h
>> @@ -4,12 +4,14 @@
>> *
>> * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
>> * Suman Anna <[email protected]>
>> + * Tero Kristo <[email protected]>
>> */
>>
>> #ifndef __LINUX_PRUSS_H
>> #define __LINUX_PRUSS_H
>>
>> #include <linux/device.h>
>> +#include <linux/err.h>
>> #include <linux/types.h>
>>
>> #define PRU_RPROC_DRVNAME "pru-rproc"
>> @@ -44,6 +46,23 @@ enum pru_ctable_idx {
>>
>> struct device_node;
>> struct rproc;
>> +struct pruss;
>> +
>> +#if IS_ENABLED(CONFIG_TI_PRUSS)
>> +
>> +struct pruss *pruss_get(struct rproc *rproc);
>> +void pruss_put(struct pruss *pruss);
>> +
>> +#else
>> +
>> +static inline struct pruss *pruss_get(struct rproc *rproc)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> +static inline void pruss_put(struct pruss *pruss) { }
>> +
>> +#endif /* CONFIG_TI_PRUSS */
>>
>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>
>> --
>> 2.25.1
>>
--
Thanks and Regards,
Danish.
On Mon, 27 Mar 2023 at 23:42, Md Danish Anwar <[email protected]> wrote:
>
> Hi Mathieu,
>
> On 28/03/23 02:28, Mathieu Poirier wrote:
> > Hi Danish
> >
> > On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
> >> From: Tero Kristo <[email protected]>
> >>
> >> Add two new get and put API, pruss_get() and pruss_put() to the
> >> PRUSS platform driver to allow client drivers to request a handle
> >> to a PRUSS device. This handle will be used by client drivers to
> >> request various operations of the PRUSS platform driver through
> >> additional API that will be added in the following patches.
> >>
> >> The pruss_get() function returns the pruss handle corresponding
> >> to a PRUSS device referenced by a PRU remoteproc instance. The
> >> pruss_put() is the complimentary function to pruss_get().
> >>
> >> Co-developed-by: Suman Anna <[email protected]>
> >> Signed-off-by: Suman Anna <[email protected]>
> >> Signed-off-by: Tero Kristo <[email protected]>
> >> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
> >> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
> >> Signed-off-by: Puranjay Mohan <[email protected]>
> >> Signed-off-by: MD Danish Anwar <[email protected]>
> >> Reviewed-by: Roger Quadros <[email protected]>
> >> ---
> >> drivers/remoteproc/pru_rproc.c | 2 +-
> >> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++-
> >> .../{pruss_driver.h => pruss_internal.h} | 7 ++-
> >> include/linux/remoteproc/pruss.h | 19 ++++++
> >> 4 files changed, 83 insertions(+), 5 deletions(-)
> >> rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
> >>
> >> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> >> index b76db7fa693d..4ddd5854d56e 100644
> >> --- a/drivers/remoteproc/pru_rproc.c
> >> +++ b/drivers/remoteproc/pru_rproc.c
> >> @@ -19,7 +19,7 @@
> >> #include <linux/of_device.h>
> >> #include <linux/of_irq.h>
> >> #include <linux/remoteproc/pruss.h>
> >> -#include <linux/pruss_driver.h>
> >> +#include <linux/pruss_internal.h>
> >> #include <linux/remoteproc.h>
> >>
> >> #include "remoteproc_internal.h"
> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> >> index 6882c86b3ce5..6c2bb02a521d 100644
> >> --- a/drivers/soc/ti/pruss.c
> >> +++ b/drivers/soc/ti/pruss.c
> >> @@ -6,6 +6,7 @@
> >> * Author(s):
> >> * Suman Anna <[email protected]>
> >> * Andrew F. Davis <[email protected]>
> >> + * Tero Kristo <[email protected]>
> >> */
> >>
> >> #include <linux/clk-provider.h>
> >> @@ -16,8 +17,9 @@
> >> #include <linux/of_address.h>
> >> #include <linux/of_device.h>
> >> #include <linux/pm_runtime.h>
> >> -#include <linux/pruss_driver.h>
> >> +#include <linux/pruss_internal.h>
> >> #include <linux/regmap.h>
> >> +#include <linux/remoteproc.h>
> >> #include <linux/slab.h>
> >>
> >> /**
> >> @@ -30,6 +32,62 @@ struct pruss_private_data {
> >> bool has_core_mux_clock;
> >> };
> >>
> >> +/**
> >> + * pruss_get() - get the pruss for a given PRU remoteproc
> >> + * @rproc: remoteproc handle of a PRU instance
> >> + *
> >> + * Finds the parent pruss device for a PRU given the @rproc handle of the
> >> + * PRU remote processor. This function increments the pruss device's refcount,
> >> + * so always use pruss_put() to decrement it back once pruss isn't needed
> >> + * anymore.
> >> + *
> >> + * Return: pruss handle on success, and an ERR_PTR on failure using one
> >> + * of the following error values
> >> + * -EINVAL if invalid parameter
> >> + * -ENODEV if PRU device or PRUSS device is not found
> >> + */
> >> +struct pruss *pruss_get(struct rproc *rproc)
> >> +{
> >> + struct pruss *pruss;
> >> + struct device *dev;
> >> + struct platform_device *ppdev;
> >> +
> >> + if (IS_ERR_OR_NULL(rproc))
> >> + return ERR_PTR(-EINVAL);
> >> +
> >
> > There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
> > or pru_rproc_get().
> >
>
> Here in this API, we are checking if rproc is NULL or not. Also we are checking
> is_pru_rproc() to make sure this rproc is pru-rproc only and not some other rproc.
>
> This API will be called from driver (icssg_prueth.c) which I'll post once this
> series is merged.
>
> In the driver we are doing,
>
> prueth->pru[slice] = pru_rproc_get(np, pru, &pruss_id);
>
> pruss = pruss_get(prueth->pru[slice]);
>
> So, before calling pruss_get() we are in fact calling pru_rproc_get() to make
> sure it's a valid rproc.
>
You are doing the right thing but because pruss_get() is exported
globally, other people eventually using the interface may not be so
rigorous. Add a comment to the pruss_get() function documentation
clearly mentioning it is expected the caller will have done a
pru_rproc_get() on @rproc.
> I think in this API, these two checks (NULL check and is_pru_rproc) should be
> OK as the driver is already calling pru_rproc_get() before this API.
>
> The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
> is_pru_rproc() will only be true if it is a "pru-rproc" implying
> pru_rproc_get() was called before calling this API.
>
> Please let me know if this is OK or if any change is required.
>
> >> + dev = &rproc->dev;
> >> +
> >> + /* make sure it is PRU rproc */
> >> + if (!dev->parent || !is_pru_rproc(dev->parent))
> >> + return ERR_PTR(-ENODEV);
> >> +
> >> + ppdev = to_platform_device(dev->parent->parent);
> >> + pruss = platform_get_drvdata(ppdev);
> >> + if (!pruss)
> >> + return ERR_PTR(-ENODEV);
> >> +
> >> + get_device(pruss->dev);
> >> +
> >> + return pruss;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pruss_get);
> >> +
> >> +/**
> >> + * pruss_put() - decrement pruss device's usecount
> >> + * @pruss: pruss handle
> >> + *
> >> + * Complimentary function for pruss_get(). Needs to be called
> >> + * after the PRUSS is used, and only if the pruss_get() succeeds.
> >> + */
> >> +void pruss_put(struct pruss *pruss)
> >> +{
> >> + if (IS_ERR_OR_NULL(pruss))
> >> + return;
> >> +
> >> + put_device(pruss->dev);
> >> +}
> >> +EXPORT_SYMBOL_GPL(pruss_put);
> >> +
> >> static void pruss_of_free_clk_provider(void *data)
> >> {
> >> struct device_node *clk_mux_np = data;
> >> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
> >> similarity index 90%
> >> rename from include/linux/pruss_driver.h
> >> rename to include/linux/pruss_internal.h
> >> index ecfded30ed05..8f91cb164054 100644
> >> --- a/include/linux/pruss_driver.h
> >> +++ b/include/linux/pruss_internal.h
> >> @@ -6,9 +6,10 @@
> >> * Suman Anna <[email protected]>
> >> */
> >>
> >> -#ifndef _PRUSS_DRIVER_H_
> >> -#define _PRUSS_DRIVER_H_
> >> +#ifndef _PRUSS_INTERNAL_H_
> >> +#define _PRUSS_INTERNAL_H_
> >>
> >> +#include <linux/remoteproc/pruss.h>
> >> #include <linux/types.h>
> >>
> >> /*
> >> @@ -51,4 +52,4 @@ struct pruss {
> >> struct clk *iep_clk_mux;
> >> };
> >>
> >> -#endif /* _PRUSS_DRIVER_H_ */
> >> +#endif /* _PRUSS_INTERNAL_H_ */
> >> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> >> index 039b50d58df2..93a98cac7829 100644
> >> --- a/include/linux/remoteproc/pruss.h
> >> +++ b/include/linux/remoteproc/pruss.h
> >> @@ -4,12 +4,14 @@
> >> *
> >> * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
> >> * Suman Anna <[email protected]>
> >> + * Tero Kristo <[email protected]>
> >> */
> >>
> >> #ifndef __LINUX_PRUSS_H
> >> #define __LINUX_PRUSS_H
> >>
> >> #include <linux/device.h>
> >> +#include <linux/err.h>
> >> #include <linux/types.h>
> >>
> >> #define PRU_RPROC_DRVNAME "pru-rproc"
> >> @@ -44,6 +46,23 @@ enum pru_ctable_idx {
> >>
> >> struct device_node;
> >> struct rproc;
> >> +struct pruss;
> >> +
> >> +#if IS_ENABLED(CONFIG_TI_PRUSS)
> >> +
> >> +struct pruss *pruss_get(struct rproc *rproc);
> >> +void pruss_put(struct pruss *pruss);
> >> +
> >> +#else
> >> +
> >> +static inline struct pruss *pruss_get(struct rproc *rproc)
> >> +{
> >> + return ERR_PTR(-EOPNOTSUPP);
> >> +}
> >> +
> >> +static inline void pruss_put(struct pruss *pruss) { }
> >> +
> >> +#endif /* CONFIG_TI_PRUSS */
> >>
> >> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
> >>
> >> --
> >> 2.25.1
> >>
>
> --
> Thanks and Regards,
> Danish.
On 29/03/23 23:29, Mathieu Poirier wrote:
> On Mon, 27 Mar 2023 at 23:42, Md Danish Anwar <[email protected]> wrote:
>>
>> Hi Mathieu,
>>
>> On 28/03/23 02:28, Mathieu Poirier wrote:
>>> Hi Danish
>>>
>>> On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
>>>> From: Tero Kristo <[email protected]>
>>>>
>>>> Add two new get and put API, pruss_get() and pruss_put() to the
>>>> PRUSS platform driver to allow client drivers to request a handle
>>>> to a PRUSS device. This handle will be used by client drivers to
>>>> request various operations of the PRUSS platform driver through
>>>> additional API that will be added in the following patches.
>>>>
>>>> The pruss_get() function returns the pruss handle corresponding
>>>> to a PRUSS device referenced by a PRU remoteproc instance. The
>>>> pruss_put() is the complimentary function to pruss_get().
>>>>
>>>> Co-developed-by: Suman Anna <[email protected]>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> Signed-off-by: Tero Kristo <[email protected]>
>>>> Co-developed-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Grzegorz Jaszczyk <[email protected]>
>>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>>> Signed-off-by: MD Danish Anwar <[email protected]>
>>>> Reviewed-by: Roger Quadros <[email protected]>
>>>> ---
>>>> drivers/remoteproc/pru_rproc.c | 2 +-
>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++-
>>>> .../{pruss_driver.h => pruss_internal.h} | 7 ++-
>>>> include/linux/remoteproc/pruss.h | 19 ++++++
>>>> 4 files changed, 83 insertions(+), 5 deletions(-)
>>>> rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>>>>
>>>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>>>> index b76db7fa693d..4ddd5854d56e 100644
>>>> --- a/drivers/remoteproc/pru_rproc.c
>>>> +++ b/drivers/remoteproc/pru_rproc.c
>>>> @@ -19,7 +19,7 @@
>>>> #include <linux/of_device.h>
>>>> #include <linux/of_irq.h>
>>>> #include <linux/remoteproc/pruss.h>
>>>> -#include <linux/pruss_driver.h>
>>>> +#include <linux/pruss_internal.h>
>>>> #include <linux/remoteproc.h>
>>>>
>>>> #include "remoteproc_internal.h"
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index 6882c86b3ce5..6c2bb02a521d 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -6,6 +6,7 @@
>>>> * Author(s):
>>>> * Suman Anna <[email protected]>
>>>> * Andrew F. Davis <[email protected]>
>>>> + * Tero Kristo <[email protected]>
>>>> */
>>>>
>>>> #include <linux/clk-provider.h>
>>>> @@ -16,8 +17,9 @@
>>>> #include <linux/of_address.h>
>>>> #include <linux/of_device.h>
>>>> #include <linux/pm_runtime.h>
>>>> -#include <linux/pruss_driver.h>
>>>> +#include <linux/pruss_internal.h>
>>>> #include <linux/regmap.h>
>>>> +#include <linux/remoteproc.h>
>>>> #include <linux/slab.h>
>>>>
>>>> /**
>>>> @@ -30,6 +32,62 @@ struct pruss_private_data {
>>>> bool has_core_mux_clock;
>>>> };
>>>>
>>>> +/**
>>>> + * pruss_get() - get the pruss for a given PRU remoteproc
>>>> + * @rproc: remoteproc handle of a PRU instance
>>>> + *
>>>> + * Finds the parent pruss device for a PRU given the @rproc handle of the
>>>> + * PRU remote processor. This function increments the pruss device's refcount,
>>>> + * so always use pruss_put() to decrement it back once pruss isn't needed
>>>> + * anymore.
>>>> + *
>>>> + * Return: pruss handle on success, and an ERR_PTR on failure using one
>>>> + * of the following error values
>>>> + * -EINVAL if invalid parameter
>>>> + * -ENODEV if PRU device or PRUSS device is not found
>>>> + */
>>>> +struct pruss *pruss_get(struct rproc *rproc)
>>>> +{
>>>> + struct pruss *pruss;
>>>> + struct device *dev;
>>>> + struct platform_device *ppdev;
>>>> +
>>>> + if (IS_ERR_OR_NULL(rproc))
>>>> + return ERR_PTR(-EINVAL);
>>>> +
>>>
>>> There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
>>> or pru_rproc_get().
>>>
>>
>> Here in this API, we are checking if rproc is NULL or not. Also we are checking
>> is_pru_rproc() to make sure this rproc is pru-rproc only and not some other rproc.
>>
>> This API will be called from driver (icssg_prueth.c) which I'll post once this
>> series is merged.
>>
>> In the driver we are doing,
>>
>> prueth->pru[slice] = pru_rproc_get(np, pru, &pruss_id);
>>
>> pruss = pruss_get(prueth->pru[slice]);
>>
>> So, before calling pruss_get() we are in fact calling pru_rproc_get() to make
>> sure it's a valid rproc.
>>
>
> You are doing the right thing but because pruss_get() is exported
> globally, other people eventually using the interface may not be so
> rigorous. Add a comment to the pruss_get() function documentation
> clearly mentioning it is expected the caller will have done a
> pru_rproc_get() on @rproc.
>
Sure, Mathieu. I will add the required comment in pruss_get() documentation.
>> I think in this API, these two checks (NULL check and is_pru_rproc) should be
>> OK as the driver is already calling pru_rproc_get() before this API.
>>
>> The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
>> is_pru_rproc() will only be true if it is a "pru-rproc" implying
>> pru_rproc_get() was called before calling this API.
>>
>> Please let me know if this is OK or if any change is required.
>>
>>>> + dev = &rproc->dev;
>>>> +
>>>> + /* make sure it is PRU rproc */
>>>> + if (!dev->parent || !is_pru_rproc(dev->parent))
>>>> + return ERR_PTR(-ENODEV);
>>>> +
>>>> + ppdev = to_platform_device(dev->parent->parent);
>>>> + pruss = platform_get_drvdata(ppdev);
>>>> + if (!pruss)
>>>> + return ERR_PTR(-ENODEV);
>>>> +
>>>> + get_device(pruss->dev);
>>>> +
>>>> + return pruss;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pruss_get);
>>>> +
>>>> +/**
>>>> + * pruss_put() - decrement pruss device's usecount
>>>> + * @pruss: pruss handle
>>>> + *
>>>> + * Complimentary function for pruss_get(). Needs to be called
>>>> + * after the PRUSS is used, and only if the pruss_get() succeeds.
>>>> + */
>>>> +void pruss_put(struct pruss *pruss)
>>>> +{
>>>> + if (IS_ERR_OR_NULL(pruss))
>>>> + return;
>>>> +
>>>> + put_device(pruss->dev);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pruss_put);
>>>> +
>>>> static void pruss_of_free_clk_provider(void *data)
>>>> {
>>>> struct device_node *clk_mux_np = data;
>>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
>>>> similarity index 90%
>>>> rename from include/linux/pruss_driver.h
>>>> rename to include/linux/pruss_internal.h
>>>> index ecfded30ed05..8f91cb164054 100644
>>>> --- a/include/linux/pruss_driver.h
>>>> +++ b/include/linux/pruss_internal.h
>>>> @@ -6,9 +6,10 @@
>>>> * Suman Anna <[email protected]>
>>>> */
>>>>
>>>> -#ifndef _PRUSS_DRIVER_H_
>>>> -#define _PRUSS_DRIVER_H_
>>>> +#ifndef _PRUSS_INTERNAL_H_
>>>> +#define _PRUSS_INTERNAL_H_
>>>>
>>>> +#include <linux/remoteproc/pruss.h>
>>>> #include <linux/types.h>
>>>>
>>>> /*
>>>> @@ -51,4 +52,4 @@ struct pruss {
>>>> struct clk *iep_clk_mux;
>>>> };
>>>>
>>>> -#endif /* _PRUSS_DRIVER_H_ */
>>>> +#endif /* _PRUSS_INTERNAL_H_ */
>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>> index 039b50d58df2..93a98cac7829 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -4,12 +4,14 @@
>>>> *
>>>> * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
>>>> * Suman Anna <[email protected]>
>>>> + * Tero Kristo <[email protected]>
>>>> */
>>>>
>>>> #ifndef __LINUX_PRUSS_H
>>>> #define __LINUX_PRUSS_H
>>>>
>>>> #include <linux/device.h>
>>>> +#include <linux/err.h>
>>>> #include <linux/types.h>
>>>>
>>>> #define PRU_RPROC_DRVNAME "pru-rproc"
>>>> @@ -44,6 +46,23 @@ enum pru_ctable_idx {
>>>>
>>>> struct device_node;
>>>> struct rproc;
>>>> +struct pruss;
>>>> +
>>>> +#if IS_ENABLED(CONFIG_TI_PRUSS)
>>>> +
>>>> +struct pruss *pruss_get(struct rproc *rproc);
>>>> +void pruss_put(struct pruss *pruss);
>>>> +
>>>> +#else
>>>> +
>>>> +static inline struct pruss *pruss_get(struct rproc *rproc)
>>>> +{
>>>> + return ERR_PTR(-EOPNOTSUPP);
>>>> +}
>>>> +
>>>> +static inline void pruss_put(struct pruss *pruss) { }
>>>> +
>>>> +#endif /* CONFIG_TI_PRUSS */
>>>>
>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>
>> --
>> Thanks and Regards,
>> Danish.
--
Thanks and Regards,
Danish.