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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux