Re: [PATCH 04/16] remoteproc/pru: Add PRU remoteproc driver

David Lechner
2018-11-26T22:32:45Z

On 11/26/18 1:52 AM, Roger Quadros wrote:

> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ce5d061..88a86cc 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -26,3 +26,4 @@ qcom_wcnss_pil-y += qcom_wcnss.o
> qcom_wcnss_pil-y += qcom_wcnss_iris.o
> obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> +obj-$(CONFIG_PRUSS_REMOTEPROC) += pru_rproc.o
> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
> new file mode 100644
> index 0000000..c35f432
> --- /dev/null
> +++ b/drivers/remoteproc/pru_rproc.c
> @@ -0,0 +1,392 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PRU-ICSS remoteproc driver for various TI SoCs
> + *
> + * Copyright (C) 2014-2018 Texas Instruments Incorporated - http://www.ti.com/
> + * Suman Anna <s-anna@ti.com>
> + * Andrew F. Davis <afd@ti.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/remoteproc.h>

alphabetical order?

> +#include <linux/pruss.h>
> +
> +#include "remoteproc_internal.h"

alphabetical order?

> +#include "pru_rproc.h"
> +
> +/* PRU_ICSS_PRU_CTRL registers */
> +#define PRU_CTRL_CTRL 0x0000
> +#define PRU_CTRL_STS 0x0004
> +#define PRU_CTRL_WAKEUP_EN 0x0008
> +#define PRU_CTRL_CYCLE 0x000C
> +#define PRU_CTRL_STALL 0x0010
> +#define PRU_CTRL_CTBIR0 0x0020
> +#define PRU_CTRL_CTBIR1 0x0024
> +#define PRU_CTRL_CTPPR0 0x0028
> +#define PRU_CTRL_CTPPR1 0x002C
> +
> +/* CTRL register bit-fields */
> +#define CTRL_CTRL_SOFT_RST_N BIT(0)
> +#define CTRL_CTRL_EN BIT(1)
> +#define CTRL_CTRL_SLEEPING BIT(2)
> +#define CTRL_CTRL_CTR_EN BIT(3)
> +#define CTRL_CTRL_SINGLE_STEP BIT(8)
> +#define CTRL_CTRL_RUNSTATE BIT(15)
> +
> +/**
> + * enum pru_mem - PRU core memory range identifiers
> + */
> +enum pru_mem {
> + PRU_MEM_IRAM = 0,
> + PRU_MEM_CTRL,
> + PRU_MEM_DEBUG,
> + PRU_MEM_MAX,
> +};

I am finding the name "mem" here to be confusing. I keep thinking
these are just RAM regions instead of memory mapped I/O. Maybe call
it "iomem" instead of "mem"?

...

> +static int pru_rproc_set_id(struct pru_rproc *pru)
> +{
> + int ret = 0;
> + u32 mask1 = 0x34000;
> + u32 mask2 = 0x38000;

These values are non-obvious and could use some comments. Also,
they could be made into constants or macros.

> +
> + if ((pru->mem_regions[0].pa & mask1) == mask1)

how about this instead:

if ((pru->mem_regions[PRU_MEM_IRAM].pa & 0xfffff) == mask1)

The 0xfffff mask will be important on AM18xx where INTC is at 0x34000,
PRU0 IRAM is at 0x38000 and PRU1 IRAM is at 0x3C000.

> + pru->id = 0;
> + else if ((pru->mem_regions[0].pa & mask2) == mask2)
> + pru->id = 1;
> + else
> + ret = -EINVAL;
> +
> + return ret;
> +}