diff mbox series

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

Message ID 20230323062451.2925996-2-danishanwar@ti.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Introduce PRU platform consumer API | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

MD Danish Anwar March 23, 2023, 6:24 a.m. UTC
From: Tero Kristo <t-kristo@ti.com>

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

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

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

Comments

Mathieu Poirier March 27, 2023, 8:58 p.m. UTC | #1
Hi Danish

On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
> From: Tero Kristo <t-kristo@ti.com>
> 
> Add two new get and put API, pruss_get() and pruss_put() to the
> PRUSS platform driver to allow client drivers to request a handle
> to a PRUSS device. This handle will be used by client drivers to
> request various operations of the PRUSS platform driver through
> additional API that will be added in the following patches.
> 
> The pruss_get() function returns the pruss handle corresponding
> to a PRUSS device referenced by a PRU remoteproc instance. The
> pruss_put() is the complimentary function to pruss_get().
> 
> Co-developed-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> ---
>  drivers/remoteproc/pru_rproc.c                |  2 +-
>  drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
>  .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
>  include/linux/remoteproc/pruss.h              | 19 ++++++
>  4 files changed, 83 insertions(+), 5 deletions(-)
>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
> 
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> index b76db7fa693d..4ddd5854d56e 100644
> --- a/drivers/remoteproc/pru_rproc.c
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -19,7 +19,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/remoteproc/pruss.h>
> -#include <linux/pruss_driver.h>
> +#include <linux/pruss_internal.h>
>  #include <linux/remoteproc.h>
>  
>  #include "remoteproc_internal.h"
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 6882c86b3ce5..6c2bb02a521d 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -6,6 +6,7 @@
>   * Author(s):
>   *	Suman Anna <s-anna@ti.com>
>   *	Andrew F. Davis <afd@ti.com>
> + *	Tero Kristo <t-kristo@ti.com>
>   */
>  
>  #include <linux/clk-provider.h>
> @@ -16,8 +17,9 @@
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/pruss_driver.h>
> +#include <linux/pruss_internal.h>
>  #include <linux/regmap.h>
> +#include <linux/remoteproc.h>
>  #include <linux/slab.h>
>  
>  /**
> @@ -30,6 +32,62 @@ struct pruss_private_data {
>  	bool has_core_mux_clock;
>  };
>  
> +/**
> + * pruss_get() - get the pruss for a given PRU remoteproc
> + * @rproc: remoteproc handle of a PRU instance
> + *
> + * Finds the parent pruss device for a PRU given the @rproc handle of the
> + * PRU remote processor. This function increments the pruss device's refcount,
> + * so always use pruss_put() to decrement it back once pruss isn't needed
> + * anymore.
> + *
> + * Return: pruss handle on success, and an ERR_PTR on failure using one
> + * of the following error values
> + *    -EINVAL if invalid parameter
> + *    -ENODEV if PRU device or PRUSS device is not found
> + */
> +struct pruss *pruss_get(struct rproc *rproc)
> +{
> +	struct pruss *pruss;
> +	struct device *dev;
> +	struct platform_device *ppdev;
> +
> +	if (IS_ERR_OR_NULL(rproc))
> +		return ERR_PTR(-EINVAL);
> +

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

> +	dev = &rproc->dev;
> +
> +	/* make sure it is PRU rproc */
> +	if (!dev->parent || !is_pru_rproc(dev->parent))
> +		return ERR_PTR(-ENODEV);
> +
> +	ppdev = to_platform_device(dev->parent->parent);
> +	pruss = platform_get_drvdata(ppdev);
> +	if (!pruss)
> +		return ERR_PTR(-ENODEV);
> +
> +	get_device(pruss->dev);
> +
> +	return pruss;
> +}
> +EXPORT_SYMBOL_GPL(pruss_get);
> +
> +/**
> + * pruss_put() - decrement pruss device's usecount
> + * @pruss: pruss handle
> + *
> + * Complimentary function for pruss_get(). Needs to be called
> + * after the PRUSS is used, and only if the pruss_get() succeeds.
> + */
> +void pruss_put(struct pruss *pruss)
> +{
> +	if (IS_ERR_OR_NULL(pruss))
> +		return;
> +
> +	put_device(pruss->dev);
> +}
> +EXPORT_SYMBOL_GPL(pruss_put);
> +
>  static void pruss_of_free_clk_provider(void *data)
>  {
>  	struct device_node *clk_mux_np = data;
> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
> similarity index 90%
> rename from include/linux/pruss_driver.h
> rename to include/linux/pruss_internal.h
> index ecfded30ed05..8f91cb164054 100644
> --- a/include/linux/pruss_driver.h
> +++ b/include/linux/pruss_internal.h
> @@ -6,9 +6,10 @@
>   *	Suman Anna <s-anna@ti.com>
>   */
>  
> -#ifndef _PRUSS_DRIVER_H_
> -#define _PRUSS_DRIVER_H_
> +#ifndef _PRUSS_INTERNAL_H_
> +#define _PRUSS_INTERNAL_H_
>  
> +#include <linux/remoteproc/pruss.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -51,4 +52,4 @@ struct pruss {
>  	struct clk *iep_clk_mux;
>  };
>  
> -#endif	/* _PRUSS_DRIVER_H_ */
> +#endif	/* _PRUSS_INTERNAL_H_ */
> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> index 039b50d58df2..93a98cac7829 100644
> --- a/include/linux/remoteproc/pruss.h
> +++ b/include/linux/remoteproc/pruss.h
> @@ -4,12 +4,14 @@
>   *
>   * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
>   *	Suman Anna <s-anna@ti.com>
> + *	Tero Kristo <t-kristo@ti.com>
>   */
>  
>  #ifndef __LINUX_PRUSS_H
>  #define __LINUX_PRUSS_H
>  
>  #include <linux/device.h>
> +#include <linux/err.h>
>  #include <linux/types.h>
>  
>  #define PRU_RPROC_DRVNAME "pru-rproc"
> @@ -44,6 +46,23 @@ enum pru_ctable_idx {
>  
>  struct device_node;
>  struct rproc;
> +struct pruss;
> +
> +#if IS_ENABLED(CONFIG_TI_PRUSS)
> +
> +struct pruss *pruss_get(struct rproc *rproc);
> +void pruss_put(struct pruss *pruss);
> +
> +#else
> +
> +static inline struct pruss *pruss_get(struct rproc *rproc)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void pruss_put(struct pruss *pruss) { }
> +
> +#endif /* CONFIG_TI_PRUSS */
>  
>  #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>  
> -- 
> 2.25.1
>
Anwar, Md Danish March 28, 2023, 5:42 a.m. UTC | #2
Hi Mathieu,

On 28/03/23 02:28, Mathieu Poirier wrote:
> Hi Danish
> 
> On Thu, Mar 23, 2023 at 11:54:47AM +0530, MD Danish Anwar wrote:
>> From: Tero Kristo <t-kristo@ti.com>
>>
>> Add two new get and put API, pruss_get() and pruss_put() to the
>> PRUSS platform driver to allow client drivers to request a handle
>> to a PRUSS device. This handle will be used by client drivers to
>> request various operations of the PRUSS platform driver through
>> additional API that will be added in the following patches.
>>
>> The pruss_get() function returns the pruss handle corresponding
>> to a PRUSS device referenced by a PRU remoteproc instance. The
>> pruss_put() is the complimentary function to pruss_get().
>>
>> Co-developed-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/remoteproc/pru_rproc.c                |  2 +-
>>  drivers/soc/ti/pruss.c                        | 60 ++++++++++++++++++-
>>  .../{pruss_driver.h => pruss_internal.h}      |  7 ++-
>>  include/linux/remoteproc/pruss.h              | 19 ++++++
>>  4 files changed, 83 insertions(+), 5 deletions(-)
>>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>>
>> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
>> index b76db7fa693d..4ddd5854d56e 100644
>> --- a/drivers/remoteproc/pru_rproc.c
>> +++ b/drivers/remoteproc/pru_rproc.c
>> @@ -19,7 +19,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/remoteproc/pruss.h>
>> -#include <linux/pruss_driver.h>
>> +#include <linux/pruss_internal.h>
>>  #include <linux/remoteproc.h>
>>  
>>  #include "remoteproc_internal.h"
>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>> index 6882c86b3ce5..6c2bb02a521d 100644
>> --- a/drivers/soc/ti/pruss.c
>> +++ b/drivers/soc/ti/pruss.c
>> @@ -6,6 +6,7 @@
>>   * Author(s):
>>   *	Suman Anna <s-anna@ti.com>
>>   *	Andrew F. Davis <afd@ti.com>
>> + *	Tero Kristo <t-kristo@ti.com>
>>   */
>>  
>>  #include <linux/clk-provider.h>
>> @@ -16,8 +17,9 @@
>>  #include <linux/of_address.h>
>>  #include <linux/of_device.h>
>>  #include <linux/pm_runtime.h>
>> -#include <linux/pruss_driver.h>
>> +#include <linux/pruss_internal.h>
>>  #include <linux/regmap.h>
>> +#include <linux/remoteproc.h>
>>  #include <linux/slab.h>
>>  
>>  /**
>> @@ -30,6 +32,62 @@ struct pruss_private_data {
>>  	bool has_core_mux_clock;
>>  };
>>  
>> +/**
>> + * pruss_get() - get the pruss for a given PRU remoteproc
>> + * @rproc: remoteproc handle of a PRU instance
>> + *
>> + * Finds the parent pruss device for a PRU given the @rproc handle of the
>> + * PRU remote processor. This function increments the pruss device's refcount,
>> + * so always use pruss_put() to decrement it back once pruss isn't needed
>> + * anymore.
>> + *
>> + * Return: pruss handle on success, and an ERR_PTR on failure using one
>> + * of the following error values
>> + *    -EINVAL if invalid parameter
>> + *    -ENODEV if PRU device or PRUSS device is not found
>> + */
>> +struct pruss *pruss_get(struct rproc *rproc)
>> +{
>> +	struct pruss *pruss;
>> +	struct device *dev;
>> +	struct platform_device *ppdev;
>> +
>> +	if (IS_ERR_OR_NULL(rproc))
>> +		return ERR_PTR(-EINVAL);
>> +
> 
> There is no guarantee that @rproc is valid without calling rproc_get_by_handle()
> or pru_rproc_get().
> 

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

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

In the driver we are doing,

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

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

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

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

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

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

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

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

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

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

>> I think in this API, these two checks (NULL check and is_pru_rproc) should be
>> OK as the driver is already calling pru_rproc_get() before this API.
>>
>> The only way to get a "pru-rproc" is by calling pru_rproc_get(), now the check
>> is_pru_rproc() will only be true if it is a "pru-rproc" implying
>> pru_rproc_get() was called before calling this API.
>>
>> Please let me know if this is OK or if any change is required.
>>
>>>> +    dev = &rproc->dev;
>>>> +
>>>> +    /* make sure it is PRU rproc */
>>>> +    if (!dev->parent || !is_pru_rproc(dev->parent))
>>>> +            return ERR_PTR(-ENODEV);
>>>> +
>>>> +    ppdev = to_platform_device(dev->parent->parent);
>>>> +    pruss = platform_get_drvdata(ppdev);
>>>> +    if (!pruss)
>>>> +            return ERR_PTR(-ENODEV);
>>>> +
>>>> +    get_device(pruss->dev);
>>>> +
>>>> +    return pruss;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pruss_get);
>>>> +
>>>> +/**
>>>> + * pruss_put() - decrement pruss device's usecount
>>>> + * @pruss: pruss handle
>>>> + *
>>>> + * Complimentary function for pruss_get(). Needs to be called
>>>> + * after the PRUSS is used, and only if the pruss_get() succeeds.
>>>> + */
>>>> +void pruss_put(struct pruss *pruss)
>>>> +{
>>>> +    if (IS_ERR_OR_NULL(pruss))
>>>> +            return;
>>>> +
>>>> +    put_device(pruss->dev);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pruss_put);
>>>> +
>>>>  static void pruss_of_free_clk_provider(void *data)
>>>>  {
>>>>      struct device_node *clk_mux_np = data;
>>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
>>>> similarity index 90%
>>>> rename from include/linux/pruss_driver.h
>>>> rename to include/linux/pruss_internal.h
>>>> index ecfded30ed05..8f91cb164054 100644
>>>> --- a/include/linux/pruss_driver.h
>>>> +++ b/include/linux/pruss_internal.h
>>>> @@ -6,9 +6,10 @@
>>>>   *  Suman Anna <s-anna@ti.com>
>>>>   */
>>>>
>>>> -#ifndef _PRUSS_DRIVER_H_
>>>> -#define _PRUSS_DRIVER_H_
>>>> +#ifndef _PRUSS_INTERNAL_H_
>>>> +#define _PRUSS_INTERNAL_H_
>>>>
>>>> +#include <linux/remoteproc/pruss.h>
>>>>  #include <linux/types.h>
>>>>
>>>>  /*
>>>> @@ -51,4 +52,4 @@ struct pruss {
>>>>      struct clk *iep_clk_mux;
>>>>  };
>>>>
>>>> -#endif      /* _PRUSS_DRIVER_H_ */
>>>> +#endif      /* _PRUSS_INTERNAL_H_ */
>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>> index 039b50d58df2..93a98cac7829 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -4,12 +4,14 @@
>>>>   *
>>>>   * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
>>>>   *  Suman Anna <s-anna@ti.com>
>>>> + *  Tero Kristo <t-kristo@ti.com>
>>>>   */
>>>>
>>>>  #ifndef __LINUX_PRUSS_H
>>>>  #define __LINUX_PRUSS_H
>>>>
>>>>  #include <linux/device.h>
>>>> +#include <linux/err.h>
>>>>  #include <linux/types.h>
>>>>
>>>>  #define PRU_RPROC_DRVNAME "pru-rproc"
>>>> @@ -44,6 +46,23 @@ enum pru_ctable_idx {
>>>>
>>>>  struct device_node;
>>>>  struct rproc;
>>>> +struct pruss;
>>>> +
>>>> +#if IS_ENABLED(CONFIG_TI_PRUSS)
>>>> +
>>>> +struct pruss *pruss_get(struct rproc *rproc);
>>>> +void pruss_put(struct pruss *pruss);
>>>> +
>>>> +#else
>>>> +
>>>> +static inline struct pruss *pruss_get(struct rproc *rproc)
>>>> +{
>>>> +    return ERR_PTR(-EOPNOTSUPP);
>>>> +}
>>>> +
>>>> +static inline void pruss_put(struct pruss *pruss) { }
>>>> +
>>>> +#endif /* CONFIG_TI_PRUSS */
>>>>
>>>>  #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>
>> --
>> Thanks and Regards,
>> Danish.
diff mbox series

Patch

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index b76db7fa693d..4ddd5854d56e 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -19,7 +19,7 @@ 
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/remoteproc/pruss.h>
-#include <linux/pruss_driver.h>
+#include <linux/pruss_internal.h>
 #include <linux/remoteproc.h>
 
 #include "remoteproc_internal.h"
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 6882c86b3ce5..6c2bb02a521d 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -6,6 +6,7 @@ 
  * Author(s):
  *	Suman Anna <s-anna@ti.com>
  *	Andrew F. Davis <afd@ti.com>
+ *	Tero Kristo <t-kristo@ti.com>
  */
 
 #include <linux/clk-provider.h>
@@ -16,8 +17,9 @@ 
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
-#include <linux/pruss_driver.h>
+#include <linux/pruss_internal.h>
 #include <linux/regmap.h>
+#include <linux/remoteproc.h>
 #include <linux/slab.h>
 
 /**
@@ -30,6 +32,62 @@  struct pruss_private_data {
 	bool has_core_mux_clock;
 };
 
+/**
+ * pruss_get() - get the pruss for a given PRU remoteproc
+ * @rproc: remoteproc handle of a PRU instance
+ *
+ * Finds the parent pruss device for a PRU given the @rproc handle of the
+ * PRU remote processor. This function increments the pruss device's refcount,
+ * so always use pruss_put() to decrement it back once pruss isn't needed
+ * anymore.
+ *
+ * Return: pruss handle on success, and an ERR_PTR on failure using one
+ * of the following error values
+ *    -EINVAL if invalid parameter
+ *    -ENODEV if PRU device or PRUSS device is not found
+ */
+struct pruss *pruss_get(struct rproc *rproc)
+{
+	struct pruss *pruss;
+	struct device *dev;
+	struct platform_device *ppdev;
+
+	if (IS_ERR_OR_NULL(rproc))
+		return ERR_PTR(-EINVAL);
+
+	dev = &rproc->dev;
+
+	/* make sure it is PRU rproc */
+	if (!dev->parent || !is_pru_rproc(dev->parent))
+		return ERR_PTR(-ENODEV);
+
+	ppdev = to_platform_device(dev->parent->parent);
+	pruss = platform_get_drvdata(ppdev);
+	if (!pruss)
+		return ERR_PTR(-ENODEV);
+
+	get_device(pruss->dev);
+
+	return pruss;
+}
+EXPORT_SYMBOL_GPL(pruss_get);
+
+/**
+ * pruss_put() - decrement pruss device's usecount
+ * @pruss: pruss handle
+ *
+ * Complimentary function for pruss_get(). Needs to be called
+ * after the PRUSS is used, and only if the pruss_get() succeeds.
+ */
+void pruss_put(struct pruss *pruss)
+{
+	if (IS_ERR_OR_NULL(pruss))
+		return;
+
+	put_device(pruss->dev);
+}
+EXPORT_SYMBOL_GPL(pruss_put);
+
 static void pruss_of_free_clk_provider(void *data)
 {
 	struct device_node *clk_mux_np = data;
diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_internal.h
similarity index 90%
rename from include/linux/pruss_driver.h
rename to include/linux/pruss_internal.h
index ecfded30ed05..8f91cb164054 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_internal.h
@@ -6,9 +6,10 @@ 
  *	Suman Anna <s-anna@ti.com>
  */
 
-#ifndef _PRUSS_DRIVER_H_
-#define _PRUSS_DRIVER_H_
+#ifndef _PRUSS_INTERNAL_H_
+#define _PRUSS_INTERNAL_H_
 
+#include <linux/remoteproc/pruss.h>
 #include <linux/types.h>
 
 /*
@@ -51,4 +52,4 @@  struct pruss {
 	struct clk *iep_clk_mux;
 };
 
-#endif	/* _PRUSS_DRIVER_H_ */
+#endif	/* _PRUSS_INTERNAL_H_ */
diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
index 039b50d58df2..93a98cac7829 100644
--- a/include/linux/remoteproc/pruss.h
+++ b/include/linux/remoteproc/pruss.h
@@ -4,12 +4,14 @@ 
  *
  * Copyright (C) 2015-2022 Texas Instruments Incorporated - http://www.ti.com
  *	Suman Anna <s-anna@ti.com>
+ *	Tero Kristo <t-kristo@ti.com>
  */
 
 #ifndef __LINUX_PRUSS_H
 #define __LINUX_PRUSS_H
 
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/types.h>
 
 #define PRU_RPROC_DRVNAME "pru-rproc"
@@ -44,6 +46,23 @@  enum pru_ctable_idx {
 
 struct device_node;
 struct rproc;
+struct pruss;
+
+#if IS_ENABLED(CONFIG_TI_PRUSS)
+
+struct pruss *pruss_get(struct rproc *rproc);
+void pruss_put(struct pruss *pruss);
+
+#else
+
+static inline struct pruss *pruss_get(struct rproc *rproc)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void pruss_put(struct pruss *pruss) { }
+
+#endif /* CONFIG_TI_PRUSS */
 
 #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)