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

[Tero Kristo <t-kristo@ti.com>]

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