diff mbox series

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

Message ID 20230306110934.2736465-4-danishanwar@ti.com (mailing list archive)
State New, archived
Headers show
Series Introduce PRU platform consumer API | expand

Commit Message

MD Danish Anwar March 6, 2023, 11:09 a.m. UTC
From: Suman Anna <s-anna@ti.com>

Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
the PRUSS platform driver to 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 <s-anna@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
---
 drivers/soc/ti/pruss.c           |  41 +++++++++++++
 include/linux/remoteproc/pruss.h | 102 +++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)

Comments

Roger Quadros March 8, 2023, 8:27 a.m. UTC | #1
Hi,

On 06/03/2023 13:09, MD Danish Anwar wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
> the PRUSS platform driver to 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 <s-anna@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> ---
>  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
Anwar, Md Danish March 8, 2023, 11:36 a.m. UTC | #2
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 <s-anna@ti.com>
>>
>> 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 <s-anna@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> ---
>>  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
Roger Quadros March 8, 2023, 11:42 a.m. UTC | #3
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 <s-anna@ti.com>
>>>
>>> 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 <s-anna@ti.com>
>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>> ---
>>>  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
>
Anwar, Md Danish March 9, 2023, 11:30 a.m. UTC | #4
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 <s-anna@ti.com>
>>>>
>>>> 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/20230210114957.2667963-3-danishanwar@ti.com/

>>
>> 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 <s-anna@ti.com>
>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>> ---
>>>>  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
>>
Anwar, Md Danish March 10, 2023, 11:53 a.m. UTC | #5
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 <s-anna@ti.com>
>>>>>
>>>>> 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/20230210114957.2667963-3-danishanwar@ti.com/
> 

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 <s-anna@ti.com>
>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>>> ---
>>>>>  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
>>>
>
Roger Quadros March 10, 2023, 1:23 p.m. UTC | #6
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 <s-anna@ti.com>
>>>>>>
>>>>>> 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/20230210114957.2667963-3-danishanwar@ti.com/
>>
> 
> 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 <s-anna@ti.com>
>>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>>>> ---
>>>>>>  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
Anwar, Md Danish March 10, 2023, 3:36 p.m. UTC | #7
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 <s-anna@ti.com>
>>>>>>>
>>>>>>> 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/20230210114957.2667963-3-danishanwar@ti.com/
>>>
>>
>> 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 <s-anna@ti.com>
>>>>>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>>>>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>>>>>> ---
>>>>>>>  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
Roger Quadros March 11, 2023, 12:06 p.m. UTC | #8
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 <s-anna@ti.com>
>>>>>>>>
>>>>>>>> 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
Anwar, Md Danish March 13, 2023, 5:01 a.m. UTC | #9
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 <s-anna@ti.com>
>>>>>>>>>
>>>>>>>>> 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
Roger Quadros March 13, 2023, 7:51 a.m. UTC | #10
On 13/03/2023 07:01, Md Danish Anwar wrote:
> 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 <s-anna@ti.com>
>>>>>>>>>>
>>>>>>>>>> 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?

Yes, that is also fine.

cheers,
-roger
diff mbox series

Patch

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)