| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1ecfd453-ecbd-cbea-605d-759ba329dd19@ti.com>
Date: Wed, 31 Jul 2019 11:28:52 -0400
From: "Andrew F. Davis" <afd@...com>
To: Tero Kristo <t-kristo@...com>, Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Santosh Shilimkar <ssantosh@...nel.org>,
Will Deacon <will.deacon@....com>,
Robin Murphy <robin.murphy@....com>,
Joerg Roedel <joro@...tes.org>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>, William Mills <wmills@...com>,
Tomi Valkeinen <tomi.valkeinen@...com>,
Sumit Semwal <sumit.semwal@...aro.org>,
John Stultz <john.stultz@...aro.org>
CC: <devicetree@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
<iommu@...ts.linux-foundation.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-media@...r.kernel.org>, <linaro-mm-sig@...ts.linaro.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] soc: ti: Add Support for the TI Page-based
Address Translator (PAT)
On 6/18/19 5:07 AM, Tero Kristo wrote:
> 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@...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...
>
There are two register spaces, the configuration space, and the actual
translation table data. I use regmap for all the configuration space.
Direct readl/writel would also work, but I prefer regmap as it lets me
work with field names vs using masks and shifts, even if it adds a
little extra code in tables at the top.
>> + 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.
>
ACK
>> +
>> 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@...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?
>
Just habit when starting a driver, but you are right it is not needed here.
>> +
>> +/* 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.
>
Will add.
>> + */
>> +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.
>
Will add.
>> +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?
>
Good catch, will fix.
> 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?
>
Simple way? No not really.. What I've been doing is allocating a
non-contiguous buffer (from system DMA-BUF heaps), writing some test
pattern to it, using this driver to convert the buffer, then sending the
new handle to our DSS (display subsystem which cannot handle
non-contiguous buffers). If all is working the test pattern is displayed
correctly.
Thanks,
Andrew
> -Tero
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Powered by blists - more mailing lists