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.
