| 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
| ||
|
Message-ID: <4b8d8b8e-9062-e4e6-770b-86bd40cc3683@kernel.org>
Date: Wed, 8 Mar 2023 13:14:29 +0200
From: Roger Quadros <rogerq@...nel.org>
To: Md Danish Anwar <a0501179@...com>,
MD Danish Anwar <danishanwar@...com>,
"Andrew F. Davis" <afd@...com>, Suman Anna <s-anna@...com>,
Vignesh Raghavendra <vigneshr@...com>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
Santosh Shilimkar <ssantosh@...nel.org>,
Nishanth Menon <nm@...com>
Cc: 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 v3 5/6] soc: ti: pruss: Add helper function
to enable OCP master ports
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@...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@...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>
>>> ---
>>> 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().
It's ok, don't make the change I suggested and leave it as it is.
>
>>> +
>>> + /* 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
Powered by blists - more mailing lists