mbox series

[v6,0/4] Introduce PRU platform consumer API

Message ID 20230331112941.823410-1-danishanwar@ti.com
Headers show
Series Introduce PRU platform consumer API | expand

Message

MD Danish Anwar March 31, 2023, 11:29 a.m. UTC
Hi All,
The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS
or simply PRUSS) on various TI SoCs consists of dual 32-bit RISC cores
(Programmable Real-Time Units, or PRUs) for program execution.

There are 3 foundation components for TI PRUSS subsystem: the PRUSS platform
driver, the PRUSS INTC driver and the PRUSS remoteproc driver. All of them have
already been merged and can be found under:
1) drivers/soc/ti/pruss.c
   Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml
2) drivers/irqchip/irq-pruss-intc.c
   Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml
3) drivers/remoteproc/pru_rproc.c
   Documentation/devicetree/bindings/remoteproc/ti,pru-consumer.yaml

The programmable nature of the PRUs provide flexibility to implement custom
peripheral interfaces, fast real-time responses, or specialized data handling.
Example of a PRU consumer drivers will be: 
  - Software UART over PRUSS
  - PRU-ICSS Ethernet EMAC

In order to make usage of common PRU resources and allow the consumer drivers 
to configure the PRU hardware for specific usage the PRU API is introduced.

This is the v6 of the old patch series [1].

Changes from v5 [1] to v6:
*) Added Reviewed by tags of Roger and Tony to the patches.
*) Added Acked by tag of Mathieu to patch 2 of this series.
*) Added NULL check for @mux in pruss_cfg_get_gpmux() API.
*) Added comment to the pruss_get() function documentation mentioning it is
expected the caller will have done a pru_rproc_get() on @rproc.
*) Fixed compilation warning "warning: ‘pruss_cfg_update’ defined but not used"
in patch 3 by squashing patch 3 [7] and patch 5 [8] of previous revision
together. Squashed patch 5 instead of patch 4 with patch 3 because patch 5 uses
both read() and update() APIs where as patch 4 only uses update() API.
Previously pruss_cfg_read()/update() APIs were intoroduced in patch 3
and used in patch 4 and 5. Now these APIs are introduced as well as used in 
patch 3.

Changes from v4 [2] to v5:
*) Addressed Roger's comment to change function argument in API 
pruss_cfg_xfr_enable(). Instead of asking user to calcualte mask, now user
will just provide the pru_type and mask will be calcualted inside the API.
*) Moved enum pru_type from pru_rproc.c to include/linux/remoteproc/pruss.h
in patch 4 / 5.
*) Moved enum pruss_gpi_mode from patch 3/5 to patch 4/5 to introduce this
enum in same patch as the API using it.
*) Moved enum pruss_gp_mux_sel from patch 3/5 to patch 5/5 to introduce this
enum in same patch as the API using it.
*) Created new headefile drivers/soc/ti/pruss.h, private to PRUSS as asked by
Roger. Moved all private definitions and pruss_cfg_read () / update ()
APIs to this newly added headerfile.
*) Renamed include/linux/pruss_driver.h to include/linux/pruss_internal.h as
suggested by Andrew and Roger.

Changes from v3 [3] to v4:
*) Added my SoB tags in all patches as earlier SoB tags were missing in few
patches.
*) Added Roger's RB tags in 3 patches.
*) Addressed Roger's comment in patch 4/5 of this series. Added check for 
   invalid GPI mode in pruss_cfg_gpimode() API.
*) Removed patch [4] from this series as that patch is no longer required.
*) Made pruss_cfg_read() and pruss_cfg_update() APIs internal to pruss.c by
   removing EXPORT_SYMBOL_GPL and making them static. Now these APIs are 
   internal to pruss.c and PRUSS CFG space is not exposed.
*) Moved APIs pruss_cfg_gpimode(), pruss_cfg_miirt_enable(), 
   pruss_cfg_xfr_enable(), pruss_cfg_get_gpmux(), pruss_cfg_set_gpmux() to
   pruss.c file as they are using APIs pruss_cfg_read / update. 
   Defined these APIs in pruss.h file as other drivers use these APIs to 
   perform respective operations.

Changes from v2 to v3:
*) No functional changes, the old series has been rebased on linux-next (tag:
next-20230306).

This series depends on another series which is already merged in the remoteproc
tree [5] and is part of v6.3-rc1. This series and the remoteproc series form 
the PRUSS consumer API which can be used by consumer drivers to utilize the 
PRUs.

One example of the consumer driver is the PRU-ICSSG ethernet driver [6],which 
depends on this series and the remoteproc series [5].

[1] https://lore.kernel.org/all/20230323062451.2925996-1-danishanwar@ti.com/
[2] https://lore.kernel.org/all/20230313111127.1229187-1-danishanwar@ti.com/
[3] https://lore.kernel.org/all/20230306110934.2736465-1-danishanwar@ti.com/
[4] https://lore.kernel.org/all/20230306110934.2736465-6-danishanwar@ti.com/
[5] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/#t
[6] https://lore.kernel.org/all/20230210114957.2667963-1-danishanwar@ti.com/
[7] https://lore.kernel.org/all/20230323062451.2925996-4-danishanwar@ti.com/
[8] https://lore.kernel.org/all/20230323062451.2925996-6-danishanwar@ti.com/

Thanks and Regards,
Md Danish Anwar

Andrew F. Davis (1):
  soc: ti: pruss: Add pruss_{request,release}_mem_region() API

Suman Anna (2):
  soc: ti: pruss: Add pruss_cfg_read()/update(),
    pruss_cfg_get_gpmux()/set_gpmux() APIs
  soc: ti: pruss: Add helper functions to set GPI mode, MII_RT_event and
    XFR

Tero Kristo (1):
  soc: ti: pruss: Add pruss_get()/put() API

 drivers/remoteproc/pru_rproc.c                |  17 +-
 drivers/soc/ti/pruss.c                        | 260 +++++++++++++++++-
 drivers/soc/ti/pruss.h                        | 112 ++++++++
 .../{pruss_driver.h => pruss_internal.h}      |  34 +--
 include/linux/remoteproc/pruss.h              | 141 ++++++++++
 5 files changed, 522 insertions(+), 42 deletions(-)
 create mode 100644 drivers/soc/ti/pruss.h
 rename include/linux/{pruss_driver.h => pruss_internal.h} (58%)

Comments

Simon Horman April 1, 2023, 2:07 p.m. UTC | #1
On Fri, Mar 31, 2023 at 04:59:40PM +0530, MD Danish Anwar wrote:
> From: Suman Anna <s-anna@ti.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@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>
> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> Reviewed-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.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@ti.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) ?

> +
> +#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.

> +
> +#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)
> +
> +#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)

...
Anwar, Md Danish April 3, 2023, 8:31 a.m. UTC | #2
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@ti.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@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>
>> Reviewed-by: Roger Quadros <rogerq@kernel.org>
>> Reviewed-by: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.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@ti.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)
> 
> ...
Simon Horman April 3, 2023, 9:01 a.m. UTC | #3
On Mon, Apr 03, 2023 at 02:01:44PM +0530, Md Danish Anwar wrote:
> 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@ti.com>

...

> 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.

Thanks Danish,

Dropping unused things seems like a good idea to me.