Re: [RFC PATCH 2/2] soc: ti: Add Support for the TI Page-based Address Translator (PAT)

On 07/06/2019 22:35, Andrew F. Davis wrote:

This patch adds a driver for the Page-based Address Translator (PAT) present on various TI SoCs. A PAT device performs address translation using tables stored in an internal SRAM. Each PAT supports a set number of pages, each occupying a programmable 4KB, 16KB, 64KB, or 1MB of addresses in a window for which an incoming transaction will be translated.

Signed-off-by: Andrew F. Davis afd@ti.com

drivers/soc/ti/Kconfig | 9 + drivers/soc/ti/Makefile | 1 + drivers/soc/ti/ti-pat.c | 569 ++++++++++++++++++++++++++++++++++++ include/uapi/linux/ti-pat.h | 44 +++ 4 files changed, 623 insertions(+) create mode 100644 drivers/soc/ti/ti-pat.c create mode 100644 include/uapi/linux/ti-pat.h

diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig index f0be35d3dcba..b838ae74d01f 100644 --- a/drivers/soc/ti/Kconfig +++ b/drivers/soc/ti/Kconfig @@ -86,4 +86,13 @@ config TI_SCI_INTA_MSI_DOMAIN help Driver to enable Interrupt Aggregator specific MSI Domain.

+config TI_PAT

• tristate "TI PAT DMA-BUF exporter"
• select REGMAP

What is the reasoning for using regmap for internal register access? Why not just use direct readl/writel for everything? To me it seems this is only used during probe time also...

• help

•  Driver for TI Page-based Address Translator (PAT). This driver
  

•  provides the an API allowing the remapping of a non-contiguous
  

•  DMA-BUF into a contiguous one that is sutable for devices needing
  

•  coniguous memory.
  

Minor typo: contiguous.

• endif # SOC_TI

diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile index b3868d392d4f..1369642b40c3 100644 --- a/drivers/soc/ti/Makefile +++ b/drivers/soc/ti/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_AMX3_PM) += pm33xx.o obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o +obj-$(CONFIG_TI_PAT) += ti-pat.o diff --git a/drivers/soc/ti/ti-pat.c b/drivers/soc/ti/ti-pat.c new file mode 100644 index 000000000000..7359ea0f7ccf --- /dev/null +++ b/drivers/soc/ti/ti-pat.c @@ -0,0 +1,569 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • TI PAT mapped DMA-BUF memory re-exporter
• • Copyright (C) 2018-2019 Texas Instruments Incorporated - http://www.ti.com/
• • Andrew F. Davis afd@ti.com
• */

+#include <linux/fs.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/mod_devicetable.h> +#include <linux/uaccess.h> +#include <linux/miscdevice.h> +#include <linux/regmap.h> +#include <linux/dma-buf.h> +#include <linux/genalloc.h> +#include <linux/vmalloc.h> +#include <linux/slab.h>

+#include <linux/ti-pat.h>

+#define TI_PAT_DRIVER_NAME "ti-pat"

Why do you have a define for this seeing it is only used in single location?

+/* TI PAT MMRS registers */ +#define TI_PAT_MMRS_PID 0x0 /* Revision Register */ +#define TI_PAT_MMRS_CONFIG 0x4 /* Config Register */ +#define TI_PAT_MMRS_CONTROL 0x10 /* Control Register */

+/* TI PAT CONTROL register field values */ +#define TI_PAT_CONTROL_ARB_MODE_UF 0x0 /* Updates first */ +#define TI_PAT_CONTROL_ARB_MODE_RR 0x2 /* Round-robin */

+#define TI_PAT_CONTROL_PAGE_SIZE_4KB 0x0 +#define TI_PAT_CONTROL_PAGE_SIZE_16KB 0x1 +#define TI_PAT_CONTROL_PAGE_SIZE_64KB 0x2 +#define TI_PAT_CONTROL_PAGE_SIZE_1MB 0x3

+static unsigned int ti_pat_page_sizes[] = {

• [TI_PAT_CONTROL_PAGE_SIZE_4KB] = 4 * 1024,
• [TI_PAT_CONTROL_PAGE_SIZE_16KB] = 16 * 1024,
• [TI_PAT_CONTROL_PAGE_SIZE_64KB] = 64 * 1024,
• [TI_PAT_CONTROL_PAGE_SIZE_1MB] = 1024 * 1024,

+};

+enum ti_pat_mmrs_fields {

• /* Revision */
• F_PID_MAJOR,
• F_PID_MINOR,
• /* Controls */
• F_CONTROL_ARB_MODE,
• F_CONTROL_PAGE_SIZE,
• F_CONTROL_REPLACE_OID_EN,
• F_CONTROL_EN,
• /* sentinel */
• F_MAX_FIELDS

+};

+static const struct reg_field ti_pat_mmrs_reg_fields[] = {

• /* Revision */
• [F_PID_MAJOR] = REG_FIELD(TI_PAT_MMRS_PID, 8, 10),
• [F_PID_MINOR] = REG_FIELD(TI_PAT_MMRS_PID, 0, 5),
• /* Controls */
• [F_CONTROL_ARB_MODE] = REG_FIELD(TI_PAT_MMRS_CONTROL, 6, 7),
• [F_CONTROL_PAGE_SIZE] = REG_FIELD(TI_PAT_MMRS_CONTROL, 4, 5),
• [F_CONTROL_REPLACE_OID_EN] = REG_FIELD(TI_PAT_MMRS_CONTROL, 1, 1),
• [F_CONTROL_EN] = REG_FIELD(TI_PAT_MMRS_CONTROL, 0, 0),

+};

+/**

• • struct ti_pat_data - PAT device instance data
• • @dev: PAT device structure
• • @mdev: misc device
• • @mmrs_map: Register map of MMRS region
• • @table_base: Base address of TABLE region

Please add kerneldoc comments for all fields.

• */

+struct ti_pat_data {

• struct device *dev;
• struct miscdevice mdev;
• struct regmap *mmrs_map;
• struct regmap_field *mmrs_fields[F_MAX_FIELDS];
• void __iomem *table_base;
• unsigned int page_count;
• unsigned int page_size;
• phys_addr_t window_base;
• struct gen_pool *pool;

+};

Kerneldoc comments for below structs would be also useful, especially for ti_pat_buffer.

+struct ti_pat_dma_buf_attachment {

• struct device *dev;
• struct sg_table *table;
• struct ti_pat_buffer *buffer;
• struct list_head list;

+};

+struct ti_pat_buffer {

• struct ti_pat_data *pat;
• struct dma_buf *i_dma_buf;
• size_t size;
• unsigned long offset;
• struct dma_buf *e_dma_buf;
• struct dma_buf_attachment *attachment;
• struct sg_table *sgt;
• struct list_head attachments;
• int map_count;
• struct mutex lock;

+};

+static const struct regmap_config ti_pat_regmap_config = {

• .reg_bits = 32,
• .val_bits = 32,
• .reg_stride = 4,

+};

+static int ti_pat_dma_buf_attach(struct dma_buf *dmabuf,

• 		 struct dma_buf_attachment *attachment)
  

+{

• struct ti_pat_dma_buf_attachment *a;
• struct ti_pat_buffer *buffer = dmabuf->priv;
• a = kzalloc(sizeof(*a), GFP_KERNEL);
• if (!a)

• return -ENOMEM;
  

• a->dev = attachment->dev;
• a->buffer = buffer;
• INIT_LIST_HEAD(&a->list);
• a->table = kzalloc(sizeof(*a->table), GFP_KERNEL);
• if (!a->table) {

• kfree(a);
  

• return -ENOMEM;
  

• }
• if (sg_alloc_table(a->table, 1, GFP_KERNEL)) {

• kfree(a->table);
  

• kfree(a);
  

• return -ENOMEM;
  

• }
• sg_set_page(a->table->sgl, pfn_to_page(PFN_DOWN(buffer->offset)), buffer->size, 0);
• attachment->priv = a;
• mutex_lock(&buffer->lock);
• /* First time attachment we attach to parent */
• if (list_empty(&buffer->attachments)) {

• buffer->attachment = dma_buf_attach(buffer->i_dma_buf, buffer->pat->dev);
  

• if (IS_ERR(buffer->attachment)) {
  

• 	dev_err(buffer->pat->dev, "Unable to attach to parent DMA-BUF\n");
  

• 	mutex_unlock(&buffer->lock);
  

• 	kfree(a->table);
  

• 	kfree(a);
  

• 	return PTR_ERR(buffer->attachment);
  

• }
  

• }
• list_add(&a->list, &buffer->attachments);
• mutex_unlock(&buffer->lock);
• return 0;

+}

+static void ti_pat_dma_buf_detatch(struct dma_buf *dmabuf,

• 		   struct dma_buf_attachment *attachment)
  

Func name should be ti_pat_dma_buf_detach instead?

Other than that, I can't see anything obvious with my limited experience with dma_buf. Is there a simple way to test this driver btw?

-Tero

-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki