netdev - Re: [EXTERNAL] Re: [PATCH v7 1/4] soc: ti: pruss: Add pruss

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b10106e9-eba7-103a-54a2-b41277af378f@ti.com>
Date:   Tue, 11 Apr 2023 17:33:42 +0530
From:   Md Danish Anwar <a0501179@...com>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        MD Danish Anwar <danishanwar@...com>
CC:     "Andrew F. Davis" <afd@...com>, Suman Anna <s-anna@...com>,
        Roger Quadros <rogerq@...nel.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        Nishanth Menon <nm@...com>, <linux-remoteproc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>,
        <srk@...com>, <devicetree@...r.kernel.org>,
        <netdev@...r.kernel.org>
Subject: Re: [EXTERNAL] Re: [PATCH v7 1/4] soc: ti: pruss: Add
 pruss_get()/put() API

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@...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@...com>
>> Signed-off-by: Suman Anna <s-anna@...com>
>> Signed-off-by: Tero Kristo <t-kristo@...com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@...aro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@...aro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@...com>
>> Reviewed-by: Roger Quadros <rogerq@...nel.org>
>> Reviewed-by: Tony Lindgren <tony@...mide.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>> ---
>>  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@ti.com/
[2] https://lore.kernel.org/all/67dd1d04-147d-0e40-c06b-6e1e0dd05f13@kernel.org/

-- 
Thanks and Regards,
Danish.