| 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: <19c82e73-2e37-582b-06aa-6f83776a562d@ti.com>
Date: Mon, 3 Apr 2023 14:01:44 +0530
From: Md Danish Anwar <a0501179@...com>
To: Simon Horman <simon.horman@...igine.com>,
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>,
Mathieu Poirier <mathieu.poirier@...aro.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 v6 3/4] soc: ti: pruss: Add
pruss_cfg_read()/update(), pruss_cfg_get_gpmux()/set_gpmux() APIs
Hi Simon,
On 01/04/23 19:37, Simon Horman wrote:
> On Fri, Mar 31, 2023 at 04:59:40PM +0530, MD Danish Anwar wrote:
>> From: Suman Anna <s-anna@...com>
>>
>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to
>> the PRUSS platform driver to read and program respectively a register
>> within the PRUSS CFG sub-module represented by a syscon driver. These
>> APIs are internal to PRUSS driver.
>>
>> Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
>> to get and set the GP MUX mode for programming the PRUSS internal wrapper
>> mux functionality as needed by usecases.
>>
>> Various useful registers and macros for certain register bit-fields and
>> their values have also been added.
>>
>> 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>
>> Reviewed-by: Roger Quadros <rogerq@...nel.org>
>> Reviewed-by: Tony Lindgren <tony@...mide.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>
> ...
>
>> diff --git a/drivers/soc/ti/pruss.h b/drivers/soc/ti/pruss.h
>> new file mode 100644
>> index 000000000000..4626d5f6b874
>> --- /dev/null
>> +++ b/drivers/soc/ti/pruss.h
>> @@ -0,0 +1,112 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * PRU-ICSS Subsystem user interfaces
>> + *
>> + * Copyright (C) 2015-2023 Texas Instruments Incorporated - http://www.ti.com
>> + * MD Danish Anwar <danishanwar@...com>
>> + */
>> +
>> +#ifndef _SOC_TI_PRUSS_H_
>> +#define _SOC_TI_PRUSS_H_
>> +
>> +#include <linux/bits.h>
>> +#include <linux/regmap.h>
>> +
>> +/*
>> + * PRU_ICSS_CFG registers
>> + * SYSCFG, ISRP, ISP, IESP, IECP, SCRP applicable on AMxxxx devices only
>> + */
>> +#define PRUSS_CFG_REVID 0x00
>> +#define PRUSS_CFG_SYSCFG 0x04
>> +#define PRUSS_CFG_GPCFG(x) (0x08 + (x) * 4)
>> +#define PRUSS_CFG_CGR 0x10
>> +#define PRUSS_CFG_ISRP 0x14
>> +#define PRUSS_CFG_ISP 0x18
>> +#define PRUSS_CFG_IESP 0x1C
>> +#define PRUSS_CFG_IECP 0x20
>> +#define PRUSS_CFG_SCRP 0x24
>> +#define PRUSS_CFG_PMAO 0x28
>> +#define PRUSS_CFG_MII_RT 0x2C
>> +#define PRUSS_CFG_IEPCLK 0x30
>> +#define PRUSS_CFG_SPP 0x34
>> +#define PRUSS_CFG_PIN_MX 0x40
>> +
>> +/* PRUSS_GPCFG register bits */
>> +#define PRUSS_GPCFG_PRU_GPO_SH_SEL BIT(25)
>> +
>> +#define PRUSS_GPCFG_PRU_DIV1_SHIFT 20
>> +#define PRUSS_GPCFG_PRU_DIV1_MASK GENMASK(24, 20)
>
> There seems to be some redundancy in the encoding of '20' above.
> I suspect this could be avoided by only defining ..._MASK
> and using it with FIELD_SET() and FIELD_PREP().
>
>> +
>> +#define PRUSS_GPCFG_PRU_DIV0_SHIFT 15
>> +#define PRUSS_GPCFG_PRU_DIV0_MASK GENMASK(15, 19)
>
> Perhaps this should be GENMASK(19, 15) ?
>
yes this should have been GENMASK(15, 19). But this macro is not used anywhere
so I'll just drop it.
>> +
>> +#define PRUSS_GPCFG_PRU_GPO_MODE BIT(14)
>> +#define PRUSS_GPCFG_PRU_GPO_MODE_DIRECT 0
>> +#define PRUSS_GPCFG_PRU_GPO_MODE_SERIAL BIT(14)
>
> Likewise, I suspect the awkwardness of using 0 to mean not BIT 14
> could be avoided through use of FIELD_SET() and FIELD_PREP().
> But maybe it doesn't help.
>
This Macro is not used anywhere in code. I'll just drop them.
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_SB BIT(13)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_SHIFT 8
>> +#define PRUSS_GPCFG_PRU_GPI_DIV1_MASK GENMASK(12, 8)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_SHIFT 3
>> +#define PRUSS_GPCFG_PRU_GPI_DIV0_MASK GENMASK(7, 3)
>> +
>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_POSITIVE 0
>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE_NEGATIVE BIT(2)
>> +#define PRUSS_GPCFG_PRU_GPI_CLK_MODE BIT(2)
>> +
All these above macros are not used anywhere in the driver code. Also in the
planned upcoming driver series, there are no APIs that will use these macros.
I'll be dropping all these redundant macros. The below macros are used in
driver so I'll keep them as it is.
>> +#define PRUSS_GPCFG_PRU_GPI_MODE_MASK GENMASK(1, 0)
>> +#define PRUSS_GPCFG_PRU_GPI_MODE_SHIFT 0
>> +
>> +#define PRUSS_GPCFG_PRU_MUX_SEL_SHIFT 26
>> +#define PRUSS_GPCFG_PRU_MUX_SEL_MASK GENMASK(29, 26)
>> +
>> +/* PRUSS_MII_RT register bits */
>> +#define PRUSS_MII_RT_EVENT_EN BIT(0)
>> +
>> +/* PRUSS_SPP register bits */
>> +#define PRUSS_SPP_XFER_SHIFT_EN BIT(1)
>> +#define PRUSS_SPP_PRU1_PAD_HP_EN BIT(0)
>> +#define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3)
>
> ...
--
Thanks and Regards,
Danish.
Powered by blists - more mailing lists