Re: [RFC PATCH 2/2] soc: ti: Add Support for the TI Page-based Address Translator (PAT)
From: Tero Kristo
Date: Tue Jun 18 2019 - 05:13:26 EST
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@xxxxxx>
---
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@xxxxxx>
+ */
+
+#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