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

From: Md Danish Anwar
Date: Thu Mar 30 2023 - 05:17:43 EST

Next message: Devi Priya: "Re: [PATCH V10 2/4] clk: qcom: Add Global Clock Controller driver for IPQ9574"
Previous message: Roan van Dijk: "[PATCH v2] ARM: stm32: add initial documentation for STM32MP151"
In reply to: Mathieu Poirier: "Re: [EXTERNAL] Re: [PATCH v5 1/5] soc: ti: pruss: Add pruss_get()/put() API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 29/03/23 23:29, Mathieu Poirier wrote:
> On Mon, 27 Mar 2023 at 23:42, Md Danish Anwar <a0501179@xxxxxx> 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@xxxxxx>
>>>>
>>>> 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@xxxxxx>
>>>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
>>>> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@xxxxxxxxxx>
>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@xxxxxxxxxx>
>>>> Signed-off-by: Puranjay Mohan <p-mohan@xxxxxx>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
>>>> Reviewed-by: Roger Quadros <rogerq@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxx>
>>>> * Andrew F. Davis <afd@xxxxxx>
>>>> + * Tero Kristo <t-kristo@xxxxxx>
>>>> */
>>>>
>>>> #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@xxxxxx>
>>>> */
>>>>
>>>> -#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@xxxxxx>
>>>> + * Tero Kristo <t-kristo@xxxxxx>
>>>> */
>>>>
>>>> #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.

Next message: Devi Priya: "Re: [PATCH V10 2/4] clk: qcom: Add Global Clock Controller driver for IPQ9574"
Previous message: Roan van Dijk: "[PATCH v2] ARM: stm32: add initial documentation for STM32MP151"
In reply to: Mathieu Poirier: "Re: [EXTERNAL] Re: [PATCH v5 1/5] soc: ti: pruss: Add pruss_get()/put() API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]