From: Md Danish Anwar <a0501179@ti.com>
To: Roger Quadros <rogerq@kernel.org>,
MD Danish Anwar <danishanwar@ti.com>,
"Andrew F. Davis" <afd@ti.com>, Suman Anna <s-anna@ti.com>,
Vignesh Raghavendra <vigneshr@ti.com>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
Bjorn Andersson <andersson@kernel.org>,
Santosh Shilimkar <ssantosh@kernel.org>,
Nishanth Menon <nm@ti.com>
Cc: <linux-remoteproc@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>,
<srk@ti.com>, <devicetree@vger.kernel.org>,
<netdev@vger.kernel.org>
Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports
Date: Wed, 8 Mar 2023 16:46:44 +0530 [thread overview]
Message-ID: <a8bbed2d-4abc-2cde-3c74-5ea7ea30b231@ti.com> (raw)
In-Reply-To: <4b8d8b8e-9062-e4e6-770b-86bd40cc3683@kernel.org>
On 08/03/23 16:44, Roger Quadros wrote:
>
>
> On 08/03/2023 13:09, Md Danish Anwar wrote:
>> Hi Roger,
>>
>> On 08/03/23 14:11, Roger Quadros wrote:
>>>
>>>
>>> On 06/03/2023 13:09, MD Danish Anwar wrote:
>>>> From: Suman Anna <s-anna@ti.com>
>>>>
>>>> The PRU-ICSS subsystem on OMAP-architecture based SoCS (AM33xx, AM437x
>>>> and AM57xx SoCs) has a control bit STANDBY_INIT in the PRUSS_CFG register
>>>> to initiate a Standby sequence (when set) and trigger a MStandby request
>>>> to the SoC's PRCM module. This same bit is also used to enable the OCP
>>>> master ports (when cleared). The clearing of the STANDBY_INIT bit requires
>>>> an acknowledgment from PRCM and is done through the monitoring of the
>>>> PRUSS_SYSCFG.SUB_MWAIT bit.
>>>>
>>>> Add a helper function pruss_cfg_ocp_master_ports() to allow the PRU
>>>> client drivers to control this bit and enable or disable the firmware
>>>> running on PRU cores access to any peripherals or memory to achieve
>>>> desired functionality. The access is disabled by default on power-up
>>>> and on any suspend (context is not maintained).
>>>>
>>>> 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 | 81 +++++++++++++++++++++++++++++++-
>>>> include/linux/remoteproc/pruss.h | 6 +++
>>>> 2 files changed, 85 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
>>>> index 537a3910ffd8..dc3abda0b8c2 100644
>>>> --- a/drivers/soc/ti/pruss.c
>>>> +++ b/drivers/soc/ti/pruss.c
>>>> @@ -22,14 +22,19 @@
>>>> #include <linux/remoteproc.h>
>>>> #include <linux/slab.h>
>>>>
>>>> +#define SYSCFG_STANDBY_INIT BIT(4)
>>>> +#define SYSCFG_SUB_MWAIT_READY BIT(5)
>>>> +
>>>> /**
>>>> * struct pruss_private_data - PRUSS driver private data
>>>> * @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
>>>> * @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
>>>> + * @has_ocp_syscfg: flag to indicate if OCP SYSCFG is present
>>>> */
>>>> struct pruss_private_data {
>>>> bool has_no_sharedram;
>>>> bool has_core_mux_clock;
>>>> + bool has_ocp_syscfg;
>>>> };
>>>>
>>>> /**
>>>> @@ -205,6 +210,72 @@ int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> }
>>>> EXPORT_SYMBOL_GPL(pruss_cfg_update);
>>>>
>>>> +/**
>>>> + * pruss_cfg_ocp_master_ports() - configure PRUSS OCP master ports
>>>> + * @pruss: the pruss instance handle
>>>> + * @enable: set to true for enabling or false for disabling the OCP master ports
>>>> + *
>>>> + * This function programs the PRUSS_SYSCFG.STANDBY_INIT bit either to enable or
>>>> + * disable the OCP master ports (applicable only on SoCs using OCP interconnect
>>>> + * like the OMAP family). Clearing the bit achieves dual functionalities - one
>>>> + * is to deassert the MStandby signal to the device PRCM, and the other is to
>>>> + * enable OCP master ports to allow accesses outside of the PRU-ICSS. The
>>>> + * function has to wait for the PRCM to acknowledge through the monitoring of
>>>> + * the PRUSS_SYSCFG.SUB_MWAIT bit when enabling master ports. Setting the bit
>>>> + * disables the master access, and also signals the PRCM that the PRUSS is ready
>>>> + * for Standby.
>>>> + *
>>>> + * Return: 0 on success, or an error code otherwise. ETIMEDOUT is returned
>>>> + * when the ready-state fails.
>>>> + */
>>>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>>>> +{
>>>> + int ret;
>>>> + u32 syscfg_val, i;
>>>> + const struct pruss_private_data *data;
>>>> +
>>>> + if (IS_ERR_OR_NULL(pruss))
>>>> + return -EINVAL;
>>>> +
>>>> + data = of_device_get_match_data(pruss->dev);
>>>> +
>>>> + /* nothing to do on non OMAP-SoCs */
>>>> + if (!data || !data->has_ocp_syscfg)
>>>> + return 0;
>>>> +
>>>> + /* assert the MStandby signal during disable path */
>>>> + if (!enable)
>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG,
>>>> + SYSCFG_STANDBY_INIT,
>>>> + SYSCFG_STANDBY_INIT);
>>>
>>> You can omit the above if() if you just encapsulate the below in
>>>
>>> if (enable) {
>>>
>>>
>> Sure, I can omit the above if() and put the below block inside if (enable) {}.
>>
>> Currently when API pruss_cfg_ocp_master_ports()is called with enable as false
>> i.e. disabling PRUSS OCP master ports is requested, we directly return
>> pruss_cfg_update() where as if we remove the above if() section and encapsulate
>> below block in if (enable) {}, then in disable scenario, call flow will
>> directly reach the label disable. In the label disable, we are updating cfg and
>> then returning "ret", but at this point the variable ret is not assigned.
>>
>> To counter this should I change the label disable to below?
>>
>> disable:
>> return pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>> SYSCFG_STANDBY_INIT);
>
> But you will loose the error code if we came here due to failure in pruss_cfg_read().
>
Yes that's why I think it's better to leave it as it is.
> It's ok, don't make the change I suggested and leave it as it is.
>
Sure
>>
>>>> +
>>>> + /* enable the OCP master ports and disable MStandby */
>>>> + ret = pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT, 0);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /* wait till we are ready for transactions - delay is arbitrary */
>>>> + for (i = 0; i < 10; i++) {
>>>> + ret = pruss_cfg_read(pruss, PRUSS_CFG_SYSCFG, &syscfg_val);
>>>> + if (ret)
>>>> + goto disable;
>>>> +
>>
>> Changing the disable label will also result in losing the return value of
>> pruss_cfg_read() API call here.
>>
>>>> + if (!(syscfg_val & SYSCFG_SUB_MWAIT_READY))
>>>> + return 0;
>>>> +
>>>> + udelay(5);
>>>> + }
>>>> +
>>>> + dev_err(pruss->dev, "timeout waiting for SUB_MWAIT_READY\n");
>>>> + ret = -ETIMEDOUT;
>>
>> Changing the disable label will also result in losing ret = -ETIMEDOUT here.
>>
>>>
>>> }
>>>
>>>> +
>>>> +disable:
>>>> + pruss_cfg_update(pruss, PRUSS_CFG_SYSCFG, SYSCFG_STANDBY_INIT,
>>>> + SYSCFG_STANDBY_INIT);
>>>> + return ret;
>>>> +}
>>
>> So should I do this modification or keep it as it is?
>>
>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_ocp_master_ports);
>>>> +
>>>> static void pruss_of_free_clk_provider(void *data)
>>>> {
>>>> struct device_node *clk_mux_np = data;
>>>> @@ -495,10 +566,16 @@ static int pruss_remove(struct platform_device *pdev)
>>>> /* instance-specific driver private data */
>>>> static const struct pruss_private_data am437x_pruss1_data = {
>>>> .has_no_sharedram = false,
>>>> + .has_ocp_syscfg = true,
>>>> };
>>>>
>>>> static const struct pruss_private_data am437x_pruss0_data = {
>>>> .has_no_sharedram = true,
>>>> + .has_ocp_syscfg = false,
>>>> +};
>>>> +
>>>> +static const struct pruss_private_data am33xx_am57xx_data = {
>>>> + .has_ocp_syscfg = true,
>>>> };
>>>
>>> How about keeping platform data for different platforms separate?
>>>
>>> i.e. am33xx_pruss_data and am57xx_pruss_data
>>>
>>
>> Sure. I'll split am33xx_am57xx_data into am33xx_pruss_data and
>> am57xx_pruss_data as well as am65x_j721e_pruss_data into am65x_pruss_data and
>> j721e_pruss_data.
>>
>>>>
>>>> static const struct pruss_private_data am65x_j721e_pruss_data = {
>>>> @@ -506,10 +583,10 @@ static const struct pruss_private_data am65x_j721e_pruss_data = {
>>>> };
>>>>
>>>> static const struct of_device_id pruss_of_match[] = {
>>>> - { .compatible = "ti,am3356-pruss" },
>>>> + { .compatible = "ti,am3356-pruss", .data = &am33xx_am57xx_data },
>>>> { .compatible = "ti,am4376-pruss0", .data = &am437x_pruss0_data, },
>>>> { .compatible = "ti,am4376-pruss1", .data = &am437x_pruss1_data, },
>>>> - { .compatible = "ti,am5728-pruss" },
>>>> + { .compatible = "ti,am5728-pruss", .data = &am33xx_am57xx_data },
>>>> { .compatible = "ti,k2g-pruss" },
>>>> { .compatible = "ti,am654-icssg", .data = &am65x_j721e_pruss_data, },
>>>> { .compatible = "ti,j721e-icssg", .data = &am65x_j721e_pruss_data, },
>>>> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
>>>> index 7952f250301a..8cb99d3cad0d 100644
>>>> --- a/include/linux/remoteproc/pruss.h
>>>> +++ b/include/linux/remoteproc/pruss.h
>>>> @@ -168,6 +168,7 @@ int pruss_release_mem_region(struct pruss *pruss,
>>>> 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);
>>>> +int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable);
>>>>
>>>> #else
>>>>
>>>> @@ -203,6 +204,11 @@ static inline int pruss_cfg_update(struct pruss *pruss, unsigned int reg,
>>>> return -EOPNOTSUPP;
>>>> }
>>>>
>>>> +static inline int pruss_cfg_ocp_master_ports(struct pruss *pruss, bool enable)
>>>> +{
>>>> + return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> #endif /* CONFIG_TI_PRUSS */
>>>>
>>>> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
>>>
>>> cheers,
>>> -roger
>>
>
> cheers,
> -roger
--
Thanks and Regards,
Danish.
next prev parent reply other threads:[~2023-03-08 11:17 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-06 11:09 [PATCH v3 0/6] Introduce PRU platform consumer API MD Danish Anwar
2023-03-06 11:09 ` [PATCH v3 1/6] soc: ti: pruss: Add pruss_get()/put() API MD Danish Anwar
2023-03-08 8:22 ` Roger Quadros
2023-03-06 11:09 ` [PATCH v3 2/6] soc: ti: pruss: Add pruss_{request,release}_mem_region() API MD Danish Anwar
2023-03-08 8:23 ` Roger Quadros
2023-03-06 11:09 ` [PATCH v3 3/6] soc: ti: pruss: Add pruss_cfg_read()/update() API MD Danish Anwar
2023-03-08 8:27 ` Roger Quadros
2023-03-08 11:36 ` [EXTERNAL] " Md Danish Anwar
2023-03-08 11:42 ` Roger Quadros
2023-03-09 11:30 ` [EXTERNAL] " Md Danish Anwar
2023-03-10 11:53 ` Md Danish Anwar
2023-03-10 13:23 ` Roger Quadros
2023-03-10 15:36 ` [EXTERNAL] " Md Danish Anwar
2023-03-11 12:06 ` Roger Quadros
2023-03-13 5:01 ` [EXTERNAL] " Md Danish Anwar
2023-03-13 7:51 ` Roger Quadros
2023-03-06 11:09 ` [PATCH v3 4/6] soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and XFR MD Danish Anwar
2023-03-08 8:34 ` Roger Quadros
2023-03-08 9:23 ` [EXTERNAL] " Md Danish Anwar
2023-03-08 11:15 ` Roger Quadros
2023-03-08 11:29 ` [EXTERNAL] " Md Danish Anwar
2023-03-08 11:39 ` Roger Quadros
2023-03-06 11:09 ` [PATCH v3 5/6] soc: ti: pruss: Add helper function to enable OCP master ports MD Danish Anwar
2023-03-08 8:41 ` Roger Quadros
2023-03-08 11:09 ` [EXTERNAL] " Md Danish Anwar
2023-03-08 11:14 ` Roger Quadros
2023-03-08 11:16 ` Md Danish Anwar [this message]
2023-03-09 7:04 ` Tony Lindgren
2023-03-09 11:34 ` [EXTERNAL] " Md Danish Anwar
2023-03-06 11:09 ` [PATCH v3 6/6] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX MD Danish Anwar
2023-03-08 8:43 ` Roger Quadros
2023-03-06 18:43 ` [PATCH v3 0/6] Introduce PRU platform consumer API Mathieu Poirier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a8bbed2d-4abc-2cde-3c74-5ea7ea30b231@ti.com \
--to=a0501179@ti.com \
--cc=afd@ti.com \
--cc=andersson@kernel.org \
--cc=danishanwar@ti.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=netdev@vger.kernel.org \
--cc=nm@ti.com \
--cc=rogerq@kernel.org \
--cc=s-anna@ti.com \
--cc=srk@ti.com \
--cc=ssantosh@kernel.org \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).