| Message ID | 20220822194721.1238-1-martin.tuma@digiteqautomotive.com |
|---|---|
| Headers | show |
| Series | Digiteq Automotive MGB4 driver | expand |
On 22/08/2022 22:47, martin.tuma@digiteqautomotive.com wrote: > From: Martin Tůma <martin.tuma@digiteqautomotive.com> > Thanks for the patch. Empty commits are not accepted, so instead you should explain here why do you need it. In general, your change should not be needed, so please explain in detail why do you think otherwise. Best regards, Krzysztof
On 22/08/2022 22:47, martin.tuma@digiteqautomotive.com wrote: > From: Martin Tůma <martin.tuma@digiteqautomotive.com> > > The driver is based on the code provided by Xilinx at > https://github.com/Xilinx/dma_ip_drivers Explain why this cannot be merged into existing Xilinx dma drivers > > There are no significant functional changes in the code except > of separating the core DMA driver functionality in a way that the code > can be used by device drivers in the kernel. Use scripts/get_maintainers.pl to CC all maintainers and relevant mailing lists. Patch will be ignored if you do not follow Linux kernel process... > > Signed-off-by: Martin Tůma <martin.tuma@digiteqautomotive.com> > --- > drivers/dma/Kconfig | 7 + > drivers/dma/xilinx/Makefile | 2 + > drivers/dma/xilinx/xdma_core.c | 3835 +++++++++++++++++++++++++++++ > drivers/dma/xilinx/xdma_core.h | 588 +++++ > drivers/dma/xilinx/xdma_thread.c | 309 +++ > drivers/dma/xilinx/xdma_thread.h | 134 + > drivers/dma/xilinx/xdma_version.h | 23 + > include/linux/dma/xilinx_xdma.h | 91 + > 8 files changed, 4989 insertions(+) > create mode 100644 drivers/dma/xilinx/xdma_core.c > create mode 100644 drivers/dma/xilinx/xdma_core.h > create mode 100644 drivers/dma/xilinx/xdma_thread.c > create mode 100644 drivers/dma/xilinx/xdma_thread.h > create mode 100644 drivers/dma/xilinx/xdma_version.h > create mode 100644 include/linux/dma/xilinx_xdma.h > > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 487ed4ddc3be..e37578a5d94e 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -793,6 +793,13 @@ config DMATEST > Simple DMA test client. Say N unless you're debugging a > DMA Device driver. > > +config XILINX_XDMA > + tristate "Xilinx XDMA Engine" > + depends on PCI > + select DMA_ENGINE > + help > + Enable support for Xilinx XDMA IP controller. > + > config DMA_ENGINE_RAID > bool > > diff --git a/drivers/dma/xilinx/Makefile b/drivers/dma/xilinx/Makefile > index 767bb45f641f..890c9c04e3c7 100644 > --- a/drivers/dma/xilinx/Makefile > +++ b/drivers/dma/xilinx/Makefile > @@ -2,3 +2,5 @@ > obj-$(CONFIG_XILINX_DMA) += xilinx_dma.o > obj-$(CONFIG_XILINX_ZYNQMP_DMA) += zynqmp_dma.o > obj-$(CONFIG_XILINX_ZYNQMP_DPDMA) += xilinx_dpdma.o > +obj-$(CONFIG_XILINX_XDMA) += xilinx_xdma.o > +xilinx_xdma-objs := xdma_core.o xdma_thread.o > diff --git a/drivers/dma/xilinx/xdma_core.c b/drivers/dma/xilinx/xdma_core.c > new file mode 100644 > index 000000000000..03f02acb5904 > --- /dev/null > +++ b/drivers/dma/xilinx/xdma_core.c > @@ -0,0 +1,3835 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This file is part of the Xilinx DMA IP Core driver for Linux > + * > + * Copyright (c) 2016-present, Xilinx, Inc. > + * Copyright (c) 2020-present, Digiteq Automotive s.r.o. > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/string.h> > +#include <linux/mm.h> > +#include <linux/errno.h> > +#include <linux/sched.h> > +#include <linux/vmalloc.h> > +#include <linux/dma/xilinx_xdma.h> > +#include "xdma_core.h" > +#include "xdma_thread.h" > +#include "xdma_version.h" > + > +#define DRV_MODULE_NAME "xdma" > +#define DRV_MODULE_DESC "Xilinx XDMA Base Driver" > +#define DRV_MODULE_RELDATE "04/2021" > + > +static char version[] = > + DRV_MODULE_DESC " " DRV_MODULE_NAME " v" DRV_MODULE_VERSION "\n"; > + > +MODULE_AUTHOR("Xilinx, Inc."); > +MODULE_DESCRIPTION(DRV_MODULE_DESC); > +MODULE_VERSION(DRV_MODULE_VERSION); > +MODULE_LICENSE("Dual BSD/GPL"); > + > +/* Module Parameters */ > +static unsigned int poll_mode; > +module_param(poll_mode, uint, 0644); > +MODULE_PARM_DESC(poll_mode, "Set 1 for hw polling, default is 0 (interrupts)"); > + > +static unsigned int interrupt_mode; > +module_param(interrupt_mode, uint, 0644); > +MODULE_PARM_DESC(interrupt_mode, "0 - Auto, 1 - MSI, 2 - MSI-x"); > + > +static unsigned int enable_credit_mp = 1; > +module_param(enable_credit_mp, uint, 0644); > +MODULE_PARM_DESC(enable_credit_mp, > + "Set 0 to disable credit feature, default is 1 (credit control enabled)"); > + > +static unsigned int desc_blen_max = XDMA_DESC_BLEN_MAX; > +module_param(desc_blen_max, uint, 0644); > +MODULE_PARM_DESC(desc_blen_max, > + "per descriptor max. buffer length, default is (1 << 28) - 1"); > + > +/* > + * xdma device management > + * maintains a list of the xdma devices > + */ > +static LIST_HEAD(xdev_list); > +static DEFINE_MUTEX(xdev_mutex); > + > +static LIST_HEAD(xdev_rcu_list); > +static DEFINE_SPINLOCK(xdev_rcu_lock); No static variables... Why do you need them? > + > +static inline int xdev_list_add(struct xdma_dev *xdev) > +{ > + mutex_lock(&xdev_mutex); > + if (list_empty(&xdev_list)) { > + xdev->idx = 0; > + if (poll_mode) { > + int rv = xdma_threads_create(xdev->h2c_channel_max + > + xdev->c2h_channel_max); > + if (rv < 0) { > + mutex_unlock(&xdev_mutex); > + return rv; > + } > + } > + } else { > + struct xdma_dev *last; > + > + last = list_last_entry(&xdev_list, struct xdma_dev, list_head); > + xdev->idx = last->idx + 1; > + } > + list_add_tail(&xdev->list_head, &xdev_list); > + mutex_unlock(&xdev_mutex); > + > + dbg_init("dev %s, xdev 0x%p, xdma idx %d.\n", > + dev_name(&xdev->pdev->dev), xdev, xdev->idx); > + > + spin_lock(&xdev_rcu_lock); > + list_add_tail_rcu(&xdev->rcu_node, &xdev_rcu_list); > + spin_unlock(&xdev_rcu_lock); > + > + return 0; > +} > + > +static inline void xdev_list_remove(struct xdma_dev *xdev) > +{ > + mutex_lock(&xdev_mutex); > + list_del(&xdev->list_head); > + if (poll_mode && list_empty(&xdev_list)) > + xdma_threads_destroy(); > + mutex_unlock(&xdev_mutex); > + > + spin_lock(&xdev_rcu_lock); > + list_del_rcu(&xdev->rcu_node); > + spin_unlock(&xdev_rcu_lock); > + synchronize_rcu(); > +} > + > +static struct xdma_dev *xdev_find_by_pdev(struct pci_dev *pdev) > +{ > + struct xdma_dev *xdev, *tmp; > + > + mutex_lock(&xdev_mutex); > + list_for_each_entry_safe(xdev, tmp, &xdev_list, list_head) { > + if (xdev->pdev == pdev) { > + mutex_unlock(&xdev_mutex); > + return xdev; > + } > + } > + mutex_unlock(&xdev_mutex); > + return NULL; > +} > + > +static inline int debug_check_dev_hndl(const char *fname, struct pci_dev *pdev, > + void *hndl) > +{ > + struct xdma_dev *xdev; > + > + if (!pdev) > + return -EINVAL; > + > + xdev = xdev_find_by_pdev(pdev); > + if (!xdev) { > + pr_info("%s pdev 0x%p, hndl 0x%p, NO match found!\n", fname, > + pdev, hndl); > + return -EINVAL; > + } > + if (xdev != hndl) { > + pr_err("%s pdev 0x%p, hndl 0x%p != 0x%p!\n", fname, pdev, hndl, > + xdev); > + return -EINVAL; > + } > + > + return 0; > +} > + > +#ifdef __LIBXDMA_DEBUG__ What is this? > +/* SECTION: Function definitions */ > +inline void __write_register(const char *fn, u32 value, void *iomem, > + unsigned long off) > +{ > + pr_err("%s: w reg 0x%lx(0x%p), 0x%x.\n", fn, off, iomem, value); > + iowrite32(value, iomem); > +} > +#define write_register(v, mem, off) __write_register(__func__, v, mem, off) > +#else > +#define write_register(v, mem, off) iowrite32(v, mem) > +#endif > + > +inline u32 read_register(void *iomem) > +{ > + return ioread32(iomem); Just use ioread32, no silly wrappers. Actually, I think it should be readl/readw/readb and so on... > +} > + > +static inline u32 build_u32(u32 hi, u32 lo) > +{ > + return ((hi & 0xFFFFUL) << 16) | (lo & 0xFFFFUL); > +} > + > +static inline u64 build_u64(u64 hi, u64 lo) > +{ > + return ((hi & 0xFFFFFFFULL) << 32) | (lo & 0xFFFFFFFFULL); > +} > + > +static void check_nonzero_interrupt_status(struct xdma_dev *xdev) > +{ > + struct interrupt_regs *reg = > + (struct interrupt_regs *)(xdev->bar[xdev->config_bar_idx] + > + XDMA_OFS_INT_CTRL); > + u32 w; > + > + w = read_register(®->user_int_enable); > + if (w) > + pr_info("%s xdma%d user_int_enable = 0x%08x\n", > + dev_name(&xdev->pdev->dev), xdev->idx, w); prints on interrupts? No, this kills dmesg. Sorry, you try to push some vendor code, instead of merging to existing drivers and using Linux coding style. I'll skip parts here - you need to work on it. :/ > +int xdma_kthread_stop(struct xdma_kthread *thp) > +{ > + int rv; > + > + if (!thp->task) { > + pr_debug_thread("kthread %s, already stopped.\n", thp->name); > + return 0; > + } > + > + thp->schedule = 1; > + rv = kthread_stop(thp->task); > + if (rv < 0) { > + pr_warn("kthread %s, stop err %d.\n", thp->name, rv); > + return rv; > + } > + > + pr_debug_thread("kthread %s, 0x%p, stopped.\n", thp->name, thp->task); > + thp->task = NULL; > + > + return 0; > +} > + > + > + Code needs cleanup... (...) > diff --git a/drivers/dma/xilinx/xdma_thread.h b/drivers/dma/xilinx/xdma_thread.h > new file mode 100644 > index 000000000000..508dd4c4c890 > --- /dev/null > +++ b/drivers/dma/xilinx/xdma_thread.h > @@ -0,0 +1,134 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This file is part of the Xilinx DMA IP Core driver for Linux > + * > + * Copyright (c) 2017-present, Xilinx, Inc. Present? There is no such year. > + */ > + > +#ifndef XDMA_THREAD_H > +#define XDMA_THREAD_H > +/** > + * @file Is it standard kerneldoc comment? > + * @brief This file contains the declarations for xdma kernel threads > + * > + */ > +#include <linux/version.h> > +#include <linux/spinlock.h> > +#include <linux/kthread.h> > +#include <linux/cpuset.h> > +#include <linux/signal.h> > +#include <linux/kernel.h> > +#include <linux/types.h> > +#include <linux/uaccess.h> > +#include <linux/errno.h> > +#include "xdma_core.h" > + > +#ifdef DEBUG_THREADS > +#define lock_thread(thp) \ > + do { \ > + pr_debug("locking thp %s ...\n", (thp)->name); \ > + spin_lock(&(thp)->lock); \ > + } while (0) > + > +#define unlock_thread(thp) \ > + do { \ > + pr_debug("unlock thp %s ...\n", (thp)->name); \ > + spin_unlock(&(thp)->lock); \ > + } while (0) > + > +#define xdma_kthread_wakeup(thp) \ > + do { \ > + pr_info("signaling thp %s ...\n", (thp)->name); \ > + wake_up_process((thp)->task); \ > + } while (0) > + > +#define pr_debug_thread(fmt, ...) pr_info(fmt, __VA_ARGS__) > + > +#else > +/** lock thread macro */ > +#define lock_thread(thp) spin_lock(&(thp)->lock) > +/** un lock thread macro */ > +#define unlock_thread(thp) spin_unlock(&(thp)->lock) > +#define xdma_kthread_wakeup(thp) \ > + do { \ > + thp->schedule = 1; \ > + wake_up_interruptible(&thp->waitq); \ > + } while (0) > +/** pr_debug_thread */ > +#define pr_debug_thread(fmt, ...) > +#endif > + > +/** > + * @struct - xdma_kthread > + * @brief xdma thread book keeping parameters > + */ > +struct xdma_kthread { > + /** thread lock*/ Weird comment style. Use Linux kernel coding style comments. In this case - use proper kerneldoc. > + spinlock_t lock; > + /** name of the thread */ > + char name[16]; > + /** cpu number for which the thread associated with */ > + unsigned short cpu; > + /** thread id */ > + unsigned short id; > + /** thread sleep timeout value */ > + unsigned int timeout; > + /** flags for thread */ > + unsigned long flag; > + /** thread wait queue */ > + wait_queue_head_t waitq; > + /* flag to indicate scheduling of thread */ > + unsigned int schedule; > + /** kernel task structure associated with thread*/ > + struct task_struct *task; > + /** thread work list count */ > + unsigned int work_cnt; > + /** thread work list count */ > + struct list_head work_list; > + /** thread initialization handler */ > + int (*finit)(struct xdma_kthread *thread); > + /** thread pending handler */ > + int (*fpending)(struct list_head *list); > + /** thread peocessing handler */ > + int (*fproc)(struct list_head *list); > + /** thread done handler */ > + int (*fdone)(struct xdma_kthread *thread); > +}; > + > + > +/*****************************************************************************/ Weird comment style. Use Linux kernel coding style comments. > +/** > + * xdma_threads_create() - create xdma threads > + *****************************************************************************/ > +int xdma_threads_create(unsigned int num_threads); > + > +/*****************************************************************************/ > +/** > + * xdma_threads_destroy() - destroy all the xdma threads created > + * during system initialization > + * > + * @return none > + *****************************************************************************/ > +void xdma_threads_destroy(void); > + > +/*****************************************************************************/ > +/** > + * xdma_thread_remove_work() - handler to remove the attached work thread > + * > + * @param[in] engine: pointer to xdma_engine > + * > + * @return none > + *****************************************************************************/ > +void xdma_thread_remove_work(struct xdma_engine *engine); > + > +/*****************************************************************************/ > +/** > + * xdma_thread_add_work() - handler to add a work thread > + * > + * @param[in] engine: pointer to xdma_engine > + * > + * @return none > + *****************************************************************************/ > +void xdma_thread_add_work(struct xdma_engine *engine); > + > +#endif /* XDMA_THREAD_H */ > diff --git a/drivers/dma/xilinx/xdma_version.h b/drivers/dma/xilinx/xdma_version.h > new file mode 100644 > index 000000000000..bd061b6bc513 > --- /dev/null > +++ b/drivers/dma/xilinx/xdma_version.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This file is part of the Xilinx DMA IP Core driver for Linux > + * > + * Copyright (c) 2016-present, Xilinx, Inc. > + */ > + > +#ifndef XDMA_VERSION_H > +#define XDMA_VERSION_H > + > +#define DRV_MOD_MAJOR 2021 > +#define DRV_MOD_MINOR 4 > +#define DRV_MOD_PATCHLEVEL 1 What's that? We do not version drivers in Linux kernel. Entire header should be removed. > + > +#define DRV_MODULE_VERSION \ > + __stringify(DRV_MOD_MAJOR) "." \ > + __stringify(DRV_MOD_MINOR) "." \ > + __stringify(DRV_MOD_PATCHLEVEL) > + > +#define DRV_MOD_VERSION_NUMBER \ > + ((DRV_MOD_MAJOR)*1000 + (DRV_MOD_MINOR)*100 + DRV_MOD_PATCHLEVEL) > + > +#endif /* XDMA_VERSION_H */ > diff --git a/include/linux/dma/xilinx_xdma.h b/include/linux/dma/xilinx_xdma.h > new file mode 100644 > index 000000000000..4a0c3e02ec6f > --- /dev/null > +++ b/include/linux/dma/xilinx_xdma.h > @@ -0,0 +1,91 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This file is part of the Xilinx DMA IP Core driver for Linux > + * > + * Copyright (c) 2016-present, Xilinx, Inc. > + * Copyright (c) 2020-present, Digiteq Automotive s.r.o. > + */ > + > +#ifndef XILINX_XDMA_H > +#define XILINX_XDMA_H > + > +#include <linux/types.h> > +#include <linux/scatterlist.h> > +#include <linux/interrupt.h> > + > +/* > + * xdma_device_open - read the pci bars and configure the fpga > + * should be called from probe() > + * NOTE: user interrupt will not enabled until xdma_user_isr_enable() is called > + * > + * @pdev: ptr to pci_dev Is it kerneldoc or not? If it is, make it proper kerneldoc. > + * @mod_name: the module name to be used for request_irq > + * @user_max: max # of user/event (interrupts) to be configured > + * @channel_max: max # of c2h and h2c channels to be configured > + * NOTE: if the user/channel provisioned is less than the max specified, > + * libxdma will update the user_max/channel_max > + * > + * returns a opaque handle (for libxdma to identify the device) NULL, in case of > + * error > + */ > +void *xdma_device_open(const char *mod_name, struct pci_dev *pdev, > + int *user_max, int *h2c_channel_max, int *c2h_channel_max); > + Best regards, Krzysztof
On 29/08/2022 14:47, Tuma, Martin (Digiteq Automotive) wrote: > > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Sunday, August 28, 2022 4:47 PM >> To: Tuma, Martin (Digiteq Automotive) <Martin.Tuma@digiteqautomotive.com>; linux-media@vger.kernel.org <linux-media@vger.kernel.org> >> Subject: Re: [PATCH 1/3] Added platform module alias for the xiic I2C driver > >>> On 22/08/2022 22:47, martin.tuma@digiteqautomotive.com wrote: >>> From: Martin Tůma <martin.tuma@digiteqautomotive.com> > >> Thanks for the patch. Empty commits are not accepted, so instead you >> should explain here why do you need it. In general, your change should >> not be needed, so please explain in detail why do you think otherwise. > > The reason the alias is required is that without the "platform" prefix, loading > the xiic module does not work properly in the mgb4 module. I can not explain > exactly why as my knowledge of the module loading mechanism in linux/modprobe > is quite limited, but that's how it is. The mgb4 v4l2 module requires two modules > that are defined using MODULE_SOFTDEP() to be loaded prior to the mgb4 > module - the Xilinx I2C module and the Xilinx SPI module. The SPI module already > has the "platform" prefixed alias and loads fine, while the I2C doesn't and does > not get loaded without it. So I added the alias to make the loading work. > > I will add the info that the alias is required by the mgb4 module to the commit > message the next time I will send the fixed patches, thanks for pointing this out. Driver matches only by Devicetree, so instead of this patch you rather miss proper DTS. Best regards, Krzysztof
On 29/08/2022 14:28, Tuma, Martin (Digiteq Automotive) wrote: > > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Sunday, August 28, 2022 4:58 PM > To: Tuma, Martin (Digiteq Automotive) <Martin.Tuma@digiteqautomotive.com>; linux-media@vger.kernel.org <linux-media@vger.kernel.org> > Subject: Re: [PATCH 2/3] Added Xilinx PCIe DMA IP core driver > > On 22/08/2022 22:47, martin.tuma@digiteqautomotive.com wrote: >> From: Martin Tůma <martin.tuma@digiteqautomotive.com> >> >> The driver is based on the code provided by Xilinx at >> https://github.com/Xilinx/dma_ip_drivers > >>> Explain why this cannot be merged into existing Xilinx dma drivers > > The Xilinx XDMA IP core is a complex device that is bound to PCIe and > also handles stuff like MSI/MSI-X interrupts of the PCIe card/FPGA. > The FPGA IP core is different from those that already have drivers in > dma/xilinx so a new dma device would be required anyway. Just because it is different does not mean it requires a new driver... > > >> >> There are no significant functional changes in the code except >> of separating the core DMA driver functionality in a way that the code >> can be used by device drivers in the kernel. > > Use scripts/get_maintainers.pl to CC all maintainers and relevant > mailing lists. Patch will be ignored if you do not follow Linux kernel > process... > > Ok, thanks for the info, I have missed this in all the "how to submit > a patch to linux" info one has to go through. I don't understand your quoting style. You typed here my message instead of quoting. I recommend to use some standard mail clients so that emails are properly formatted. Best regards, Krzysztof
>>> Thanks for the patch. Empty commits are not accepted, so instead you >>> should explain here why do you need it. In general, your change should >>> not be needed, so please explain in detail why do you think otherwise. >> >> The reason the alias is required is that without the "platform" prefix, loading >> the xiic module does not work properly in the mgb4 module. I can not explain >> exactly why as my knowledge of the module loading mechanism in linux/modprobe >> is quite limited, but that's how it is. The mgb4 v4l2 module requires two modules >> that are defined using MODULE_SOFTDEP() to be loaded prior to the mgb4 >> module - the Xilinx I2C module and the Xilinx SPI module. The SPI module already >> has the "platform" prefixed alias and loads fine, while the I2C doesn't and does >> not get loaded without it. So I added the alias to make the loading work. >> >> I will add the info that the alias is required by the mgb4 module to the commit >> message the next time I will send the fixed patches, thanks for pointing this out. > Driver matches only by Devicetree, so instead of this patch you rather > miss proper DTS. Can you please explain this in more depth? There is AFAIK no device tree on x86 and I have no idea how this should work for a PCIe card on ARM either. The fact really is, that on x86_64 and ARM (Nvidia jetson) without any specific devicetree where I tested the driver, the mgb4 driver loads properly both the I2C and SPI modules defined using MODULE_SOFTDEP (there is no link dependency) if and only if they are defined using the "platform" prefix (and the module has that alias, hence this patch). So there must IMHO be some mechanism in the kernel or in modprobe, that works based on the prefix. M. INTERNAL
On 30/08/2022 20:56, Tuma, Martin (Digiteq Automotive) wrote: > > >>>> Thanks for the patch. Empty commits are not accepted, so instead you >>>> should explain here why do you need it. In general, your change should >>>> not be needed, so please explain in detail why do you think otherwise. >>> >>> The reason the alias is required is that without the "platform" prefix, loading >>> the xiic module does not work properly in the mgb4 module. I can not explain >>> exactly why as my knowledge of the module loading mechanism in linux/modprobe >>> is quite limited, but that's how it is. The mgb4 v4l2 module requires two modules >>> that are defined using MODULE_SOFTDEP() to be loaded prior to the mgb4 >>> module - the Xilinx I2C module and the Xilinx SPI module. The SPI module already >>> has the "platform" prefixed alias and loads fine, while the I2C doesn't and does >>> not get loaded without it. So I added the alias to make the loading work. >>> >>> I will add the info that the alias is required by the mgb4 module to the commit >>> message the next time I will send the fixed patches, thanks for pointing this out. > >> Driver matches only by Devicetree, so instead of this patch you rather >> miss proper DTS. > > Can you please explain this in more depth? There is AFAIK no device tree on x86 > and I have no idea how this should work for a PCIe card on ARM either. Ah, right, you do not use it for DT platform. Then you need proper ID table, e.g. for ACPI. platform_device_id table would also do the trick but I don't think it is suitable for such matching via ACPI. > > The fact really is, that on x86_64 and ARM (Nvidia jetson) without any specific devicetree > where I tested the driver, the mgb4 driver loads properly both the I2C and SPI modules > defined using MODULE_SOFTDEP (there is no link dependency) if and only if they are > defined using the "platform" prefix (and the module has that alias, hence this patch). So > there must IMHO be some mechanism in the kernel or in modprobe, that works based > on the prefix. Nvidia Jetson is ARM (and not an ACPI?) so it comes with DT. Let's don't mix problems. Depending on the type of your system where this is used, you need proper matching. Sprinkling aliases is not the way, usually. Best regards, Krzysztof
On 31/08/2022 17:12, Tuma, Martin (Digiteq Automotive) wrote: > > >>> The Xilinx XDMA IP core is a complex device that is bound to PCIe and >>> also handles stuff like MSI/MSI-X interrupts of the PCIe card/FPGA. >>> The FPGA IP core is different from those that already have drivers in >>> dma/xilinx so a new dma device would be required anyway. >> >> Just because it is different does not mean it requires a new driver... > > Just because the HW is from Xilinx and has DMA in it's name does not mean > it can be merged with some other Xilinx DMA driver... In many cases it means because submitters are not willing to integrate but prefer to duplicate... > I suggest we stop this kind > of argumentation as it is pointless and we look at the facts. I was waiting for the facts - for the actual differences. > The XDMA IP core > really is very different from the other three Xilinx DMA engines which already have > a driver in linux. Additionally as you can see, there are three supported Xilinx > DMA engines and each of them has its own driver, so I see no reason for > breaking this "rule" and try to violently merge the XDMA driver with one of > the existing drivers (their maintainers would IMHO not be very happy...) There is no rule that you need new driver for every new IP block. It depends. Anyway, I raised the concerns. You will get them probably again from other people when you Cc proper addresses... > >> I don't understand your quoting style. You typed here my message instead >> of quoting. I recommend to use some standard mail clients so that emails >> are properly formatted. > > The story behind the weird quoting style is, I only have a web Outlook accesible > through Citrix, where even copy&paste does not work... This is how things are if > you work for the Volkswagen/Škoda corporate (Digiteq is a subsidiary of Škoda) > like I do. The official mail addresses and their infrastructure is simply unusable for > "serious" work. I even had to set up my own SMTP server in the Internet to actually > send the patches... I will switch to different email address the next time I send > the reworked patches and use some sane email client. There were two reasons for > using the broken mail infrastructure: Sorry to hear that. Usually the only viable solution is to keep discussions and submits with other (e.g. private and working) accounts/setups. Git works fine with it, only authorship of emails is different. Of course company might prohibit such approach... > > 1) By using the "official" company mail address I wanted to make clear that the driver > is developed by the company producing the HW. Author of email does not have to be the same as author of commit. > 2) I didn't know that the web Outlook is that bad and only designed for the "corporate" > style of replyies where you post your response on top of the previous message. > > TLDR - sorry for the "style", it will get better the next time I send the patches. Best regards, Krzysztof
> Ah, right, you do not use it for DT platform. Then you need proper ID > table, e.g. for ACPI. platform_device_id table would also do the trick > but I don't think it is suitable for such matching via ACPI. The mgb4 driver of course uses the propper device id table (the PCI id) and matches and loads fine. The problem is, it needs two other modules to be loaded prior to it, where one of them is the xiic module. It is used by a platform device that gets created/instantiated during the mgb4 inicialization. As there is no symbol dependency, the dependency between the modules can only be defined using MODULE_SOFTDEP. And for modprobe to work correctly you need the platform alias. >> The fact really is, that on x86_64 and ARM (Nvidia jetson) without any specific devicetree >> where I tested the driver, the mgb4 driver loads properly both the I2C and SPI modules >> defined using MODULE_SOFTDEP (there is no link dependency) if and only if they are >> defined using the "platform" prefix (and the module has that alias, hence this patch). So >> there must IMHO be some mechanism in the kernel or in modprobe, that works based >> on the prefix. > > Nvidia Jetson is ARM (and not an ACPI?) so it comes with DT. Let's don't > mix problems. Depending on the type of your system where this is used, > you need proper matching. Sprinkling aliases is not the way, usually. This is not problem mixing. You really can not expect every user to define a DT for a PCI Express card that he may or may not use! The type of the system is irrelevant here, a PCIe card has to work based on the PCI id and not some additional mechanism like DT or ACPI you suggest. The problem this patch is solving is the inter-module dependency (mgb4 requires xiic to be loaded). If you think that this inter-module dependency should be solved differently, then please provide _how exactly_ this should be done, not some hypotetic solutions for problems that we do not have like some platform dependency of the drivers, and I will rewrite the patches. Otherwise I really do not see any reason for your fight agains this one line patch, that adds an alias that many other drivers (like the second one we are using in mgb4 - the Xilinx SPI driver) already have and that actually solves the problem. M. INTERNAL
On 31/08/2022 17:44, Tuma, Martin (Digiteq Automotive) wrote: > > >> Ah, right, you do not use it for DT platform. Then you need proper ID >> table, e.g. for ACPI. platform_device_id table would also do the trick >> but I don't think it is suitable for such matching via ACPI. > > The mgb4 driver of course uses the propper device id table (the PCI id) and > matches and loads fine. The problem is, it needs two other modules to be loaded > prior to it, where one of them is the xiic module. It is used by a platform device > that gets created/instantiated during the mgb4 inicialization. As there is no symbol > dependency, the dependency between the modules can only be defined using > MODULE_SOFTDEP. And for modprobe to work correctly you need the platform > alias. I don't know what is mgb4 - there is nothing like that in the sources (git grep, find). Other existing devices instantiate it via MFD child device, which works on platform devices and this points to the need of platform_device_id table. The commit could then indicate as fix for: Fixes: b822039b8ec1 ("i2c: xiic: Fix coding style issues") I also don't understand the reason for alias removal in that commit - "none is really using it" - because at least one driver (timberdale) uses it... > >>> The fact really is, that on x86_64 and ARM (Nvidia jetson) without any specific devicetree >>> where I tested the driver, the mgb4 driver loads properly both the I2C and SPI modules >>> defined using MODULE_SOFTDEP (there is no link dependency) if and only if they are >>> defined using the "platform" prefix (and the module has that alias, hence this patch). So >>> there must IMHO be some mechanism in the kernel or in modprobe, that works based >>> on the prefix. >> >> Nvidia Jetson is ARM (and not an ACPI?) so it comes with DT. Let's don't >> mix problems. Depending on the type of your system where this is used, >> you need proper matching. Sprinkling aliases is not the way, usually. > > This is not problem mixing. You really can not expect every user to define a DT > for a PCI Express card that he may or may not use! The type of the system is > irrelevant here, a PCIe card has to work based on the PCI id and not some > additional mechanism like DT or ACPI you suggest. The type of system is relevant because from that piece you start analyzing the problem. I have no clue which piece added such device in your system (ACPI tables? DTB) and you failed to provide such information. > The problem this patch is solving is the inter-module dependency (mgb4 > requires xiic to be loaded). If you think that this inter-module dependency should > be solved differently, then please provide _how exactly_ this should be done, I already said - proper device tables. > not > some hypotetic solutions for problems that we do not have like some platform > dependency of the drivers, and I will rewrite the patches. Otherwise I really do not > see any reason for your fight agains this one line patch, that adds an alias that > many other drivers (like the second one we are using in mgb4 - the Xilinx SPI > driver) already have and that actually solves the problem. Aliases are hiding the actual user and binding method leading to commit like b822039b8ec1 saying - no one uses it. You need to implement proper matching method (e.g. platform device table), not sprinkle aliases. Just because some other driver chosen poor way it is not argument to repeat it... Best regards, Krzysztof
From: Martin Tůma <martin.tuma@digiteqautomotive.com> Hi, This series of patches adds a driver for the Digiteq Automotive MGB4 grabber card. MGB4 is a modular frame grabber PCIe card for automotive video interfaces (FPD-Link and GMSL for now). It is based on a Xilinx FPGA and uses their XDMA IP core for DMA transfers. Additionally, Xilinx I2C and SPI IP cores which already have drivers in linux are used in the design. Except of the required xiic driver alias, the patches are split into two parts: the XDMA driver and a "standard" v4l2 device driver. The XDMA driver is based on Xilinx's sample code (https://github.com/Xilinx/dma_ip_drivers) with minor modifications making it usable for further FPGA-based PCIe cards. The driver was put under DMA clients in the tree, but I'm really not sure here, if that is ok and if the driver is "good enaugh" as it is... The rest is a quite standard v4l2 driver, with one exception - there are a lot of sysfs options that may/must be set before opening the v4l2 device to adapt the card on a specific signal (see mgb4-sysfs.rst for details) as the card must be able to work with various signal sources (or displays) that can not be auto-detected. I have run the driver through the v4l2-compliance test suite for both the input and the output and the results look fine to me (I can provide the output if required). Martin Tůma, Digiteq Automotive Martin Tůma (3): Added platform module alias for the xiic I2C driver Added Xilinx PCIe DMA IP core driver Added Digiteq Automotive MGB4 driver Documentation/admin-guide/media/mgb4-iio.rst | 30 + Documentation/admin-guide/media/mgb4-mtd.rst | 16 + .../admin-guide/media/mgb4-sysfs.rst | 297 ++ drivers/dma/Kconfig | 7 + drivers/dma/xilinx/Makefile | 2 + drivers/dma/xilinx/xdma_core.c | 3835 +++++++++++++++++ drivers/dma/xilinx/xdma_core.h | 588 +++ drivers/dma/xilinx/xdma_thread.c | 309 ++ drivers/dma/xilinx/xdma_thread.h | 134 + drivers/dma/xilinx/xdma_version.h | 23 + drivers/i2c/busses/i2c-xiic.c | 1 + drivers/media/pci/Kconfig | 1 + drivers/media/pci/Makefile | 1 + drivers/media/pci/mgb4/Kconfig | 17 + drivers/media/pci/mgb4/Makefile | 6 + drivers/media/pci/mgb4/mgb4_cmt.c | 243 ++ drivers/media/pci/mgb4/mgb4_cmt.h | 16 + drivers/media/pci/mgb4/mgb4_core.c | 512 +++ drivers/media/pci/mgb4/mgb4_core.h | 49 + drivers/media/pci/mgb4/mgb4_i2c.c | 139 + drivers/media/pci/mgb4/mgb4_i2c.h | 35 + drivers/media/pci/mgb4/mgb4_io.h | 36 + drivers/media/pci/mgb4/mgb4_regs.c | 30 + drivers/media/pci/mgb4/mgb4_regs.h | 35 + drivers/media/pci/mgb4/mgb4_sysfs.h | 18 + drivers/media/pci/mgb4/mgb4_sysfs_in.c | 750 ++++ drivers/media/pci/mgb4/mgb4_sysfs_out.c | 734 ++++ drivers/media/pci/mgb4/mgb4_sysfs_pci.c | 83 + drivers/media/pci/mgb4/mgb4_trigger.c | 200 + drivers/media/pci/mgb4/mgb4_trigger.h | 8 + drivers/media/pci/mgb4/mgb4_vin.c | 649 +++ drivers/media/pci/mgb4/mgb4_vin.h | 64 + drivers/media/pci/mgb4/mgb4_vout.c | 496 +++ drivers/media/pci/mgb4/mgb4_vout.h | 58 + include/linux/dma/xilinx_xdma.h | 91 + 35 files changed, 9513 insertions(+) create mode 100644 Documentation/admin-guide/media/mgb4-iio.rst create mode 100644 Documentation/admin-guide/media/mgb4-mtd.rst create mode 100644 Documentation/admin-guide/media/mgb4-sysfs.rst create mode 100644 drivers/dma/xilinx/xdma_core.c create mode 100644 drivers/dma/xilinx/xdma_core.h create mode 100644 drivers/dma/xilinx/xdma_thread.c create mode 100644 drivers/dma/xilinx/xdma_thread.h create mode 100644 drivers/dma/xilinx/xdma_version.h create mode 100644 drivers/media/pci/mgb4/Kconfig create mode 100644 drivers/media/pci/mgb4/Makefile create mode 100644 drivers/media/pci/mgb4/mgb4_cmt.c create mode 100644 drivers/media/pci/mgb4/mgb4_cmt.h create mode 100644 drivers/media/pci/mgb4/mgb4_core.c create mode 100644 drivers/media/pci/mgb4/mgb4_core.h create mode 100644 drivers/media/pci/mgb4/mgb4_i2c.c create mode 100644 drivers/media/pci/mgb4/mgb4_i2c.h create mode 100644 drivers/media/pci/mgb4/mgb4_io.h create mode 100644 drivers/media/pci/mgb4/mgb4_regs.c create mode 100644 drivers/media/pci/mgb4/mgb4_regs.h create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs.h create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_in.c create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_out.c create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_pci.c create mode 100644 drivers/media/pci/mgb4/mgb4_trigger.c create mode 100644 drivers/media/pci/mgb4/mgb4_trigger.h create mode 100644 drivers/media/pci/mgb4/mgb4_vin.c create mode 100644 drivers/media/pci/mgb4/mgb4_vin.h create mode 100644 drivers/media/pci/mgb4/mgb4_vout.c create mode 100644 drivers/media/pci/mgb4/mgb4_vout.h create mode 100644 include/linux/dma/xilinx_xdma.h