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

[Md Danish Anwar]

Hi Mathieu

On 10/04/23 22:39, Mathieu Poirier wrote:
> On Tue, Apr 04, 2023 at 05:23:33PM +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>
>> Reviewed-by: Roger Quadros <rogerq@xxxxxxxxxx>
>> Reviewed-by: Tony Lindgren <tony@xxxxxxxxxxx>
>> Signed-off-by: MD Danish Anwar <danishanwar@xxxxxx>
>> ---
>>  drivers/remoteproc/pru_rproc.c                |  2 +-
>>  drivers/soc/ti/pruss.c                        | 64 ++++++++++++++++++-
>>  .../{pruss_driver.h => pruss_internal.h}      |  7 +-
>>  include/linux/remoteproc/pruss.h              | 19 ++++++
>>  4 files changed, 87 insertions(+), 5 deletions(-)
>>  rename include/linux/{pruss_driver.h => pruss_internal.h} (90%)
>>
> 
> Throughout this patchset an API to access resources required by the PRUSS
> is added to pruss.c but all the function declarations are added to
> remoteproc/pruss.h.  Is this something you were asked to do or is this how the
> original implementation was?
> 

Previously in v3 [1] remoteproc/pruss.h had pruss_get() / pruss_put(),
pruss_request_mem_region() / pruss_release_mem_region() and pruss_cfg_read() /
update() API declaration in it. These APIs were defined in pruss.c. The APIs
were defined in pruss.c.

The other APIs [pruss_cfg_get_gpmux()/set(), pruss_cfg_gpimode,
pruss_cfg_miirt_enable, pruss_cfg_xfr_enable] were declared as well as defined
in linux/pruss_driver.h.

But in order to make pruss_cfg_read() / update() API internal to PRUSS as asked
by Roger, the other APIs' [pruss_cfg_get_gpmux()/set(), pruss_cfg_gpimode,
pruss_cfg_miirt_enable, pruss_cfg_xfr_enable] declaration was moved to pruss.c
and definition was moved to remoteproc/pruss.h and pruss_cfg_read() / update()
API's declaration as well as definition was moved to drivers/soc/ti/pruss.h

So now remoteproc/pruss.h has declaration of below APIs.
	pruss_get
	pruss_put
	pruss_request_mem_region
	pruss_release_mem_region
	pruss_cfg_get_gpmux
	pruss_cfg_set_gpmux
	pruss_cfg_gpimode
	pruss_cfg_miirt_enable
	pruss_cfg_xfr_enable

All these APIs are defined in drivers/soc/ti/pruss.c

> Other than pruss_get() nothing in there is related to the remoteproc
> subsystem, the bulk of the work is all about PRUSS.
> 
> In my opinion all the function declaration should go in pruss_driver.h, which
> should stay as it is and not made internal.  The code looks good now but it
> needs to be added where it belongs.
>> Thanks,
> Mathieu
> 

There was discussion on moving these declaration to pruss_driver.h but Andrew
and Roger agreed on keeping these in remoteproc/pruss.h only in v4 [2]. Also
Andrew suggested in v4 to rename pruss_driver.h to internal.

Please let me know if I should keep them as it is or try to move APIs from
remoteproc/pruss.h to pruss_driver.h.

Earlier in v3 below APIs were there in remoteproc/pruss.h
	pruss_get
	pruss_put
	pruss_request_mem_region
	pruss_release_mem_region

Should I keep all these in remoteproc/pruss.h or only pruss_get/put() as
mentioned by you in this email.

[1] https://lore.kernel.org/all/20230306110934.2736465-1-danishanwar@xxxxxx/
[2] https://lore.kernel.org/all/67dd1d04-147d-0e40-c06b-6e1e0dd05f13@xxxxxxxxxx/

-- 
Thanks and Regards,
Danish.