- Linaro-mm-sig - lists.linaro.org

Re: [PATCH v2 2/2] net: ethernet: ti: am65-cpsw: Add minimal XDP support

by Andrew Lunn

On Tue, Mar 05, 2024 at 11:46:00AM +0100, Julien Panis wrote: > On 3/1/24 17:38, Andrew Lunn wrote: > > On Fri, Mar 01, 2024 at 04:02:53PM +0100, Julien Panis wrote: > > > This patch adds XDP (eXpress Data Path) support to TI AM65 CPSW > > > Ethernet driver. The following features are implemented: > > > - NETDEV_XDP_ACT_BASIC (XDP_PASS, XDP_TX, XDP_DROP, XDP_ABORTED) > > > - NETDEV_XDP_ACT_REDIRECT (XDP_REDIRECT) > > > - NETDEV_XDP_ACT_NDO_XMIT (ndo_xdp_xmit callback) > > > > > > The page pool memory model is used to get better performance. > > Do you have any benchmark numbers? It should help with none XDP > > traffic as well. So maybe iperf numbers before and after? > > > > Andrew > > Argh...Houston, we have a problem. I checked my v3, which is ready for > submission, with iperf3: > 1) Before = without page pool -> 500 MBits/sec > 2) After = with page pool -> 442 MBits/sec > -> ~ 10% worse with page pool here. > > Unless the difference is not due to page pool. Maybe there's something else > which is not good in my patch. I'm going to send the v3 which uses page pool, > hopefully someone will find out something suspicious. Meanwhile, I'll carry on > investigating: I'll check the results with my patch, by removing only the using of > page pool. You can also go the other way. First add page pool support. For the FEC, that improved its performance. Then add XDP, which i think decreased the performance a little. It is extra processing in the hot path, so a little loss is not unsurprising. What tends to be expensive with ARM is cache invalidation and flush. So make sure you have the lengths correct. You don't want to operate on more memory than necessary. No point flushing the full MTU for a 64 byte TCP ACK, etc. Andrew

1 year, 9 months

1
0
0 0

Re: [PATCH v7 0/6] iio: new DMABUF based API

by Paul Cercueil

Hi Jonathan, Le mardi 05 mars 2024 à 10:07 +0000, Jonathan Cameron a écrit : > On Mon, 04 Mar 2024 08:59:47 +0100 > Nuno Sá <noname.nuno(a)gmail.com> wrote: > > > On Sun, 2024-03-03 at 17:42 +0000, Jonathan Cameron wrote: > > > On Fri, 23 Feb 2024 13:13:58 +0100 > > > Nuno Sa <nuno.sa(a)analog.com> wrote: > > > > > > > Hi Jonathan, likely you're wondering why I'm sending v7. Well, > > > > to be > > > > honest, we're hoping to get this merged this for the 6.9 merge > > > > window. > > > > Main reason is because the USB part is already in (so it would > > > > be nice > > > > to get the whole thing in). Moreover, the changes asked in v6 > > > > were simple > > > > (even though I'm not quite sure in one of them) and Paul has no > > > > access to > > > > it's laptop so he can't send v7 himself. So he kind of > > > > said/asked for me to > > > > do it. > > > > > > So, we are cutting this very fine. If Linus hints strongly at an > > > rc8 maybe we > > > can sneak this in. However, I need an Ack from Vinod for the dma > > > engine > > > changes first. > > > > > > Also I'd love a final 'looks ok' comment from DMABUF folk (Ack > > > even better!) > > > > > > Seems that the other side got resolved in the USB gadget, but > > > last we heard > > > form > > > Daniel and Christian looks to have been back on v5. I'd like them > > > to confirm > > > they are fine with the changes made as a result. > > > > > > > I can ask Christian or Daniel for some acks but my feeling (I still > > need, at > > some point, to get really familiar with all of this) is that this > > should be > > pretty similar to the USB series (from a DMABUF point of view) as > > they are both > > importers. > > > > > I've been happy with the IIO parts for a few versions now but my > > > ability to > > > review > > > the DMABUF and DMA engine bits is limited. > > > > > > A realistic path to get this in is rc8 is happening, is all Acks > > > in place by > > > Wednesday, > > > I get apply it and hits Linux-next Thursday, Pull request to Greg > > > on Saturday > > > and Greg > > > is feeling particularly generous to take one on the day he > > > normally closes his > > > trees. > > > > > > > Well, it looks like we still have a shot. I'll try to see if Vinod > > is fine with > > the DMAENGINE stuff. > > > > Sadly, looks like rc7 was at the end of a quiet week, so almost > certain to not > be an rc8 in the end. Let's aim to get this in at the start of the > next cycle > so we can build on it from there. And it looks like I'll need a V8 for the few things noted by Christian. Having it in 6.9 would have been great but having it eventually merged is all that matters - so I'm fine to have it queued for 6.10 instead. Cheers, -Paul

1 year, 9 months

1
0
0 0

Re: ECC memory semantics for heaps

by John Stultz

On Mon, Mar 4, 2024 at 5:46 AM Maxime Ripard <mripard(a)redhat.com> wrote: > On Wed, Feb 28, 2024 at 08:17:55PM -0800, John Stultz wrote: > > On Wed, Feb 28, 2024 at 7:24 AM Maxime Ripard <mripard(a)redhat.com> wrote: > > > > > > I'm currently working on a platform that seems to have togglable RAM ECC > > > support. Enabling ECC reduces the memory capacity and memory bandwidth, > > > so while it's a good idea to protect most of the system, it's not worth > > > it for things like framebuffers that won't really be affected by a > > > bitflip. > > > > > > It's currently setup by enabling ECC on the entire memory, and then > > > having a region of memory where ECC is disabled and where we're supposed > > > to allocate from for allocations that don't need it. > > > > > > My first thought to support this was to create a reserved memory region > > > for the !ECC memory, and to create a heap to allocate buffers from that > > > region. That would leave the system protected by ECC, while enabling > > > userspace to be nicer to the system by allocating buffers from the !ECC > > > region if it doesn't need it. > > > > > > However, this creates basically a new combination compared to the one we > > > already have (ie, physically contiguous vs virtually contiguous), and we > > > probably would want to throw in cacheable vs non-cacheable too. > > > > > > If we had to provide new heaps for each variation, we would have 8 heaps > > > (and 6 new ones), which could be fine I guess but would still increase > > > quite a lot the number of heaps we have so far. > > > > > > Is it something that would be a problem? If it is, do you see another > > > way to support those kind of allocations (like providing hints through > > > the ioctl maybe?)? > > > > So, the dma-buf heaps interface uses chardevs so that we can have a > > lot of flexibility in the types of heaps (and don't have the risk of > > bitmask exhaustion like ION had). So I don't see adding many > > differently named heaps as particularly problematic. > > Ok > > > That said, if there are truly generic properties (cacheable vs > > non-cachable is maybe one of those) which apply to most heaps, I'm > > open to making use of the flags. But I want to avoid having per-heap > > flags, it really needs to be a generic attribute. > > > > And I personally don't mind the idea of having things added as heaps > > initially, and potentially upgrading them to flags if needed (allowing > > heap drivers to optionally enumerate the old chardevs behind a config > > option for backwards compatibility). > > > > How common is the hardware that is going to provide this configurable > > ECC option > > In terms of number of SoCs with the feature, it's probably a handful. In > terms of number of units shipped, we're in the fairly common range :) > Sure, I guess I was trying to get a sense of is this a feature we'll likely be seeing commonly across hardware (such that internal kernel allocators would be considering it as a flag), or is it more tied to a single vendor such that enabling/isolating it in a driver is the right place in the abstraction to put it. > > and will you really want the option on all of the heap types? > > Aside from the cacheable/uncacheable discussion, yes. We could probably > get away with only physically contiguous allocations at the moment > though, I'll double check. Ok, that will be useful to know. > > Will there be any hardware constraint limitations caused by the > > ECC/!ECC flags? (ie: Devices that can't use !ECC allocated buffers?) > > My understanding is that there's no device restriction. It will be a > carved out memory so we will need to maintain a separate pool and it > will be limited in size, but that's pretty much the only one afaik. Ok. > > If not, I wonder if it would make sense to have something more along > > the lines using a fcntl() like how F_SEAL_* is used with memfds? > > With some of the discussion around "restricted"/"secure" heaps that > > can change state, I've liked this idea of just allocating dmabufs from > > normal heaps and then using fcntl or something similar to modify > > properties of the buffer that are separate from the type of memory > > that is needed to be allocated to satisfy device constraints. > > Sorry, I'm not super familiar with F_SEAL so I don't really follow what > you have in mind here. Do you have any additional resources I could read > to understand better what you're thinking about? See the File Sealing section: https://man7.org/linux/man-pages/man2/fcntl.2.html > Also, if we were to modify the ECC attributes after the dma-buf has been > allocated by dma-buf, and if the !ECC memory is carved out only, then > wouldn't that mean we would need to reallocate the backing buffer for > that dma-buf? So yea, having to work on a larger pool likely makes this not useful here, so apologies for the tangent. To explain myself, part of what I'm thinking of is, the dmabuf heaps (and really ION before it) try to solve how to allocate a buffer type that can be used across a number of devices that may have different constraints. So the focus was on "types of memory" to satisfy the constraint (contiguous, non-contiguous, secure/restricted, etc), which come down to what pages actually get used. However, outside of the "constraint type" the buffer may have, there are other "properties" that may affect performance (like cacheability, and some variants of "restricted buffers" which can change over their lifetime). With ION vendors would mix these two together in their vendor heaps, and with out-of-tree dmabuf heaps it is also common to tangle types and properties together. So I'm sort of stewing on how to best distinguish between heaps for "types of memory/pages" (ie: what's *required* to share the buffer between devices) vs these buffer properties (which affect performance) that may apply to multiple memory types. (And I'm also not 100% convinced that distinguishing between this is necessary, but casually mixing them feels messy to me) For buffers where those properties might change over time (like some variants of restricted buffers), I think the fnctl/F_SEAL_* idea makes sense to allow the buffer to become restricted. For cacheability, it seems likely an allocation flag would be nicest, but we don't have upstream users and not a lot of heap types yet, thus the out-of-tree "system-uncached" heap which sort of mixes types and properties. With ECC I was trying to get a sense of where it would sit between this "type of memory" vs a "buffer property". If internal allocators are likely to consider it in a way similar to CMA (and with the pool granular control, it sounds like it), then yeah, it probably should be a type of memory, so a new heap name is likely the way to go - but there is still the question of how to best support the various combinations of (contiguous, cacheable) along with ECC. But if it were something that was dynamically controllable at a finer grained level in the future, maybe it would be something like a buffer property. thanks -john

1 year, 9 months

1
0
0 0

Re: [PATCH v7 3/6] iio: core: Add new DMABUF interface infrastructure

by Christian König

Am 23.02.24 um 13:14 schrieb Nuno Sa: > From: Paul Cercueil <paul(a)crapouillou.net> > > Add the necessary infrastructure to the IIO core to support a new > optional DMABUF based interface. > > With this new interface, DMABUF objects (externally created) can be > attached to a IIO buffer, and subsequently used for data transfer. > > A userspace application can then use this interface to share DMABUF > objects between several interfaces, allowing it to transfer data in a > zero-copy fashion, for instance between IIO and the USB stack. > > The userspace application can also memory-map the DMABUF objects, and > access the sample data directly. The advantage of doing this vs. the > read() interface is that it avoids an extra copy of the data between the > kernel and userspace. This is particularly userful for high-speed > devices which produce several megabytes or even gigabytes of data per > second. > > As part of the interface, 3 new IOCTLs have been added: > > IIO_BUFFER_DMABUF_ATTACH_IOCTL(int fd): > Attach the DMABUF object identified by the given file descriptor to the > buffer. > > IIO_BUFFER_DMABUF_DETACH_IOCTL(int fd): > Detach the DMABUF object identified by the given file descriptor from > the buffer. Note that closing the IIO buffer's file descriptor will > automatically detach all previously attached DMABUF objects. > > IIO_BUFFER_DMABUF_ENQUEUE_IOCTL(struct iio_dmabuf *): > Request a data transfer to/from the given DMABUF object. Its file > descriptor, as well as the transfer size and flags are provided in the > "iio_dmabuf" structure. > > These three IOCTLs have to be performed on the IIO buffer's file > descriptor, obtained using the IIO_BUFFER_GET_FD_IOCTL() ioctl. A few nit picks and one bug below, apart from that looks good to me. > > Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> > Signed-off-by: Nuno Sa <nuno.sa(a)analog.com> > --- > drivers/iio/industrialio-buffer.c | 462 ++++++++++++++++++++++++++++++++++++++ > include/linux/iio/buffer_impl.h | 33 +++ > include/uapi/linux/iio/buffer.h | 22 ++ > 3 files changed, 517 insertions(+) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index b581a7e80566..0e63a09fa90a 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -9,14 +9,19 @@ > * - Better memory allocation techniques? > * - Alternative access techniques? > */ > +#include <linux/atomic.h> > #include <linux/anon_inodes.h> > #include <linux/kernel.h> > #include <linux/export.h> > #include <linux/device.h> > +#include <linux/dma-buf.h> > +#include <linux/dma-fence.h> > +#include <linux/dma-resv.h> > #include <linux/file.h> > #include <linux/fs.h> > #include <linux/cdev.h> > #include <linux/slab.h> > +#include <linux/mm.h> > #include <linux/poll.h> > #include <linux/sched/signal.h> > > @@ -28,6 +33,32 @@ > #include <linux/iio/buffer.h> > #include <linux/iio/buffer_impl.h> > > +#define DMABUF_ENQUEUE_TIMEOUT_MS 5000 > + > +struct iio_dma_fence; > + > +struct iio_dmabuf_priv { > + struct list_head entry; > + struct kref ref; > + > + struct iio_buffer *buffer; > + struct iio_dma_buffer_block *block; > + > + u64 context; > + spinlock_t lock; > + > + struct dma_buf_attachment *attach; > + struct sg_table *sgt; > + enum dma_data_direction dir; > + atomic_t seqno; > +}; > + > +struct iio_dma_fence { > + struct dma_fence base; > + struct iio_dmabuf_priv *priv; > + struct work_struct work; > +}; > + > static const char * const iio_endian_prefix[] = { > [IIO_BE] = "be", > [IIO_LE] = "le", > @@ -332,6 +363,8 @@ void iio_buffer_init(struct iio_buffer *buffer) > { > INIT_LIST_HEAD(&buffer->demux_list); > INIT_LIST_HEAD(&buffer->buffer_list); > + INIT_LIST_HEAD(&buffer->dmabufs); > + mutex_init(&buffer->dmabufs_mutex); > init_waitqueue_head(&buffer->pollq); > kref_init(&buffer->ref); > if (!buffer->watermark) > @@ -1519,14 +1552,62 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev) > kfree(iio_dev_opaque->legacy_scan_el_group.attrs); > } > > +static void iio_buffer_dmabuf_release(struct kref *ref) > +{ > + struct iio_dmabuf_priv *priv = container_of(ref, struct iio_dmabuf_priv, ref); > + struct dma_buf_attachment *attach = priv->attach; > + struct iio_buffer *buffer = priv->buffer; > + struct dma_buf *dmabuf = attach->dmabuf; > + > + dma_resv_lock(dmabuf->resv, NULL); > + dma_buf_unmap_attachment(attach, priv->sgt, priv->dir); > + dma_resv_unlock(dmabuf->resv); > + > + buffer->access->detach_dmabuf(buffer, priv->block); > + > + dma_buf_detach(attach->dmabuf, attach); > + dma_buf_put(dmabuf); > + kfree(priv); > +} > + > +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach) > +{ > + struct iio_dmabuf_priv *priv = attach->importer_priv; > + > + kref_get(&priv->ref); > +} > +EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_get); > + > +void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach) > +{ > + struct iio_dmabuf_priv *priv = attach->importer_priv; > + > + kref_put(&priv->ref, iio_buffer_dmabuf_release); > +} > +EXPORT_SYMBOL_GPL(iio_buffer_dmabuf_put); > + > static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep) > { > struct iio_dev_buffer_pair *ib = filep->private_data; > struct iio_dev *indio_dev = ib->indio_dev; > struct iio_buffer *buffer = ib->buffer; > + struct iio_dmabuf_priv *priv, *tmp; > > wake_up(&buffer->pollq); > > + mutex_lock(&buffer->dmabufs_mutex); > + > + /* Close all attached DMABUFs */ > + list_for_each_entry_safe(priv, tmp, &buffer->dmabufs, entry) { > + list_del_init(&priv->entry); > + iio_buffer_dmabuf_put(priv->attach); > + } > + > + if (!list_empty(&buffer->dmabufs)) > + dev_warn(&indio_dev->dev, "Buffer FD closed with active transfers\n"); That check here smells fishy. Either the list_for_each_entry_safe() above has removed all entries and then it can never happen or the buffer list is not always properly protected by the mutex. Do you really need that? If yes then please justify. BTW: When the transfers are async it's perfectly possible that there are active transfers on close (when the application is just killed for example). > + > + mutex_unlock(&buffer->dmabufs_mutex); > + > kfree(ib); > clear_bit(IIO_BUSY_BIT_POS, &buffer->flags); > iio_device_put(indio_dev); > @@ -1534,11 +1615,391 @@ static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep) > return 0; > } > > +static int iio_dma_resv_lock(struct dma_buf *dmabuf, bool nonblock) > +{ > + if (!nonblock) > + return dma_resv_lock_interruptible(dmabuf->resv, NULL); > + > + if (!dma_resv_trylock(dmabuf->resv)) > + return -EBUSY; > + > + return 0; > +} > + > +static struct dma_buf_attachment * > +iio_buffer_find_attachment(struct iio_dev_buffer_pair *ib, > + struct dma_buf *dmabuf, bool nonblock) > +{ > + struct device *dev = ib->indio_dev->dev.parent; > + struct iio_buffer *buffer = ib->buffer; > + struct dma_buf_attachment *attach = NULL; > + struct iio_dmabuf_priv *priv; > + > + mutex_lock(&buffer->dmabufs_mutex); > + > + list_for_each_entry(priv, &buffer->dmabufs, entry) { > + if (priv->attach->dev == dev > + && priv->attach->dmabuf == dmabuf) { > + attach = priv->attach; > + break; > + } > + } > + > + if (attach) > + iio_buffer_dmabuf_get(attach); > + > + mutex_unlock(&buffer->dmabufs_mutex); > + > + return attach ?: ERR_PTR(-EPERM); > +} > + > +static int iio_buffer_attach_dmabuf(struct iio_dev_buffer_pair *ib, > + int __user *user_fd, bool nonblock) > +{ > + struct iio_dev *indio_dev = ib->indio_dev; > + struct iio_buffer *buffer = ib->buffer; > + struct dma_buf_attachment *attach; > + struct iio_dmabuf_priv *priv; > + struct dma_buf *dmabuf; > + int err, fd; > + > + if (!buffer->access->attach_dmabuf > + || !buffer->access->detach_dmabuf > + || !buffer->access->enqueue_dmabuf) > + return -EPERM; > + > + if (copy_from_user(&fd, user_fd, sizeof(fd))) > + return -EFAULT; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + spin_lock_init(&priv->lock); > + priv->context = dma_fence_context_alloc(1); > + > + dmabuf = dma_buf_get(fd); > + if (IS_ERR(dmabuf)) { > + err = PTR_ERR(dmabuf); > + goto err_free_priv; > + } > + > + attach = dma_buf_attach(dmabuf, indio_dev->dev.parent); > + if (IS_ERR(attach)) { > + err = PTR_ERR(attach); > + goto err_dmabuf_put; > + } > + > + err = iio_dma_resv_lock(dmabuf, nonblock); > + if (err) > + goto err_dmabuf_detach; > + > + priv->dir = buffer->direction == IIO_BUFFER_DIRECTION_IN > + ? DMA_FROM_DEVICE : DMA_TO_DEVICE; > + > + priv->sgt = dma_buf_map_attachment(attach, priv->dir); > + if (IS_ERR(priv->sgt)) { > + err = PTR_ERR(priv->sgt); > + dev_err(&indio_dev->dev, "Unable to map attachment: %d\n", err); > + goto err_resv_unlock; > + } > + > + kref_init(&priv->ref); > + priv->buffer = buffer; > + priv->attach = attach; > + attach->importer_priv = priv; > + > + priv->block = buffer->access->attach_dmabuf(buffer, attach); > + if (IS_ERR(priv->block)) { > + err = PTR_ERR(priv->block); > + goto err_dmabuf_unmap_attachment; > + } > + > + dma_resv_unlock(dmabuf->resv); > + > + mutex_lock(&buffer->dmabufs_mutex); > + list_add(&priv->entry, &buffer->dmabufs); > + mutex_unlock(&buffer->dmabufs_mutex); Is it valid to attach the same dma_buf multiple times? If not that code here should probably check that. > + > + return 0; > + > +err_dmabuf_unmap_attachment: > + dma_buf_unmap_attachment(attach, priv->sgt, priv->dir); > +err_resv_unlock: > + dma_resv_unlock(dmabuf->resv); > +err_dmabuf_detach: > + dma_buf_detach(dmabuf, attach); > +err_dmabuf_put: > + dma_buf_put(dmabuf); > +err_free_priv: > + kfree(priv); > + > + return err; > +} > + > +static int iio_buffer_detach_dmabuf(struct iio_dev_buffer_pair *ib, > + int *user_req, bool nonblock) > +{ > + struct iio_buffer *buffer = ib->buffer; > + struct iio_dev *indio_dev = ib->indio_dev; > + struct iio_dmabuf_priv *priv; > + struct dma_buf *dmabuf; > + int dmabuf_fd, ret = -EPERM; ENOENT is more common to use when something can't be found I think, but perfectly up to you what you prefer. > + > + if (copy_from_user(&dmabuf_fd, user_req, sizeof(dmabuf_fd))) > + return -EFAULT; > + > + dmabuf = dma_buf_get(dmabuf_fd); > + if (IS_ERR(dmabuf)) > + return PTR_ERR(dmabuf); > + > + mutex_lock(&buffer->dmabufs_mutex); > + > + list_for_each_entry(priv, &buffer->dmabufs, entry) { > + if (priv->attach->dev == indio_dev->dev.parent > + && priv->attach->dmabuf == dmabuf) { > + list_del(&priv->entry); > + > + /* Unref the reference from iio_buffer_attach_dmabuf() */ > + iio_buffer_dmabuf_put(priv->attach); > + ret = 0; > + break; > + } > + } > + > + mutex_unlock(&buffer->dmabufs_mutex); > + dma_buf_put(dmabuf); > + > + return ret; > +} > + > +static const char * > +iio_buffer_dma_fence_get_driver_name(struct dma_fence *fence) > +{ > + return "iio"; > +} > + > +static void iio_buffer_dma_fence_release(struct dma_fence *fence) > +{ > + struct iio_dma_fence *iio_fence = > + container_of(fence, struct iio_dma_fence, base); > + > + kfree(iio_fence); > +} > + > +static const struct dma_fence_ops iio_buffer_dma_fence_ops = { > + .get_driver_name = iio_buffer_dma_fence_get_driver_name, > + .get_timeline_name = iio_buffer_dma_fence_get_driver_name, > + .release = iio_buffer_dma_fence_release, > +}; > + > +static int iio_buffer_enqueue_dmabuf(struct iio_dev_buffer_pair *ib, > + struct iio_dmabuf __user *iio_dmabuf_req, > + bool nonblock) > +{ > + struct iio_buffer *buffer = ib->buffer; > + struct iio_dmabuf iio_dmabuf; > + struct dma_buf_attachment *attach; > + struct iio_dmabuf_priv *priv; > + struct iio_dma_fence *fence; > + struct dma_buf *dmabuf; > + unsigned long timeout; > + bool cookie, cyclic, dma_to_ram; > + long retl; > + u32 seqno; > + int ret; > + > + if (copy_from_user(&iio_dmabuf, iio_dmabuf_req, sizeof(iio_dmabuf))) > + return -EFAULT; > + > + if (iio_dmabuf.flags & ~IIO_BUFFER_DMABUF_SUPPORTED_FLAGS) > + return -EINVAL; > + > + cyclic = iio_dmabuf.flags & IIO_BUFFER_DMABUF_CYCLIC; > + > + /* Cyclic flag is only supported on output buffers */ > + if (cyclic && buffer->direction != IIO_BUFFER_DIRECTION_OUT) > + return -EINVAL; > + > + dmabuf = dma_buf_get(iio_dmabuf.fd); > + if (IS_ERR(dmabuf)) > + return PTR_ERR(dmabuf); > + > + if (!iio_dmabuf.bytes_used || iio_dmabuf.bytes_used > dmabuf->size) { > + ret = -EINVAL; > + goto err_dmabuf_put; > + } > + > + attach = iio_buffer_find_attachment(ib, dmabuf, nonblock); > + if (IS_ERR(attach)) { > + ret = PTR_ERR(attach); > + goto err_dmabuf_put; > + } > + > + priv = attach->importer_priv; > + > + fence = kmalloc(sizeof(*fence), GFP_KERNEL); > + if (!fence) { > + ret = -ENOMEM; > + goto err_attachment_put; > + } > + > + fence->priv = priv; > + > + seqno = atomic_add_return(1, &priv->seqno); > + > + /* > + * The transfers are guaranteed to be processed in the order they are > + * enqueued, so we can use a simple incrementing sequence number for > + * the dma_fence. > + */ > + dma_fence_init(&fence->base, &iio_buffer_dma_fence_ops, > + &priv->lock, priv->context, seqno); > + > + ret = iio_dma_resv_lock(dmabuf, nonblock); > + if (ret) > + goto err_fence_put; > + > + timeout = nonblock ? 0 : msecs_to_jiffies(DMABUF_ENQUEUE_TIMEOUT_MS); > + > + /* Make sure we don't have writers */ > + retl = dma_resv_wait_timeout(dmabuf->resv, DMA_RESV_USAGE_WRITE, > + true, timeout); > + if (retl == 0) > + retl = -EBUSY; > + if (retl < 0) { > + ret = (int)retl; > + goto err_resv_unlock; > + } > + > + dma_to_ram = buffer->direction == IIO_BUFFER_DIRECTION_IN; > + > + if (dma_to_ram) { > + /* > + * If we're writing to the DMABUF, make sure we don't have > + * readers > + */ > + retl = dma_resv_wait_timeout(dmabuf->resv, > + DMA_RESV_USAGE_READ, true, > + timeout); > + if (retl == 0) > + retl = -EBUSY; > + if (retl < 0) { > + ret = (int)retl; > + goto err_resv_unlock; > + } > + } > + > + if (buffer->access->lock_queue) > + buffer->access->lock_queue(buffer); > + > + ret = dma_resv_reserve_fences(dmabuf->resv, 1); > + if (ret) > + goto err_queue_unlock; > + > + dma_resv_add_fence(dmabuf->resv, &fence->base, > + dma_resv_usage_rw(dma_to_ram)); That is incorrect use of the function dma_resv_usage_rw(). That function is for retrieving fences and not adding them. See the function implementation and comments, when you use it like this you get exactly what you don't want. Regards, Christian. > + dma_resv_unlock(dmabuf->resv); > + > + cookie = dma_fence_begin_signalling(); > + > + ret = buffer->access->enqueue_dmabuf(buffer, priv->block, &fence->base, > + priv->sgt, iio_dmabuf.bytes_used, > + cyclic); > + if (ret) { > + /* > + * DMABUF enqueue failed, but we already added the fence. > + * Signal the error through the fence completion mechanism. > + */ > + iio_buffer_signal_dmabuf_done(&fence->base, ret); > + } > + > + if (buffer->access->unlock_queue) > + buffer->access->unlock_queue(buffer); > + > + dma_fence_end_signalling(cookie); > + dma_buf_put(dmabuf); > + > + return ret; > + > +err_queue_unlock: > + if (buffer->access->unlock_queue) > + buffer->access->unlock_queue(buffer); > +err_resv_unlock: > + dma_resv_unlock(dmabuf->resv); > +err_fence_put: > + dma_fence_put(&fence->base); > +err_attachment_put: > + iio_buffer_dmabuf_put(attach); > +err_dmabuf_put: > + dma_buf_put(dmabuf); > + > + return ret; > +} > + > +static void iio_buffer_cleanup(struct work_struct *work) > +{ > + struct iio_dma_fence *fence = > + container_of(work, struct iio_dma_fence, work); > + struct iio_dmabuf_priv *priv = fence->priv; > + struct dma_buf_attachment *attach = priv->attach; > + > + dma_fence_put(&fence->base); > + iio_buffer_dmabuf_put(attach); > +} > + > +void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret) > +{ > + struct iio_dma_fence *iio_fence = > + container_of(fence, struct iio_dma_fence, base); > + bool cookie = dma_fence_begin_signalling(); > + > + /* > + * Get a reference to the fence, so that it's not freed as soon as > + * it's signaled. > + */ > + dma_fence_get(fence); > + > + fence->error = ret; > + dma_fence_signal(fence); > + dma_fence_end_signalling(cookie); > + > + /* > + * The fence will be unref'd in iio_buffer_cleanup. > + * It can't be done here, as the unref functions might try to lock the > + * resv object, which can deadlock. > + */ > + INIT_WORK(&iio_fence->work, iio_buffer_cleanup); > + schedule_work(&iio_fence->work); > +} > +EXPORT_SYMBOL_GPL(iio_buffer_signal_dmabuf_done); > + > +static long iio_buffer_chrdev_ioctl(struct file *filp, > + unsigned int cmd, unsigned long arg) > +{ > + struct iio_dev_buffer_pair *ib = filp->private_data; > + void __user *_arg = (void __user *)arg; > + bool nonblock = filp->f_flags & O_NONBLOCK; > + > + switch (cmd) { > + case IIO_BUFFER_DMABUF_ATTACH_IOCTL: > + return iio_buffer_attach_dmabuf(ib, _arg, nonblock); > + case IIO_BUFFER_DMABUF_DETACH_IOCTL: > + return iio_buffer_detach_dmabuf(ib, _arg, nonblock); > + case IIO_BUFFER_DMABUF_ENQUEUE_IOCTL: > + return iio_buffer_enqueue_dmabuf(ib, _arg, nonblock); > + default: > + return -EINVAL; > + } > +} > + > static const struct file_operations iio_buffer_chrdev_fileops = { > .owner = THIS_MODULE, > .llseek = noop_llseek, > .read = iio_buffer_read, > .write = iio_buffer_write, > + .unlocked_ioctl = iio_buffer_chrdev_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > .poll = iio_buffer_poll, > .release = iio_buffer_chrdev_release, > }; > @@ -1953,6 +2414,7 @@ static void iio_buffer_release(struct kref *ref) > { > struct iio_buffer *buffer = container_of(ref, struct iio_buffer, ref); > > + mutex_destroy(&buffer->dmabufs_mutex); > buffer->access->release(buffer); > } > > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h > index 89c3fd7c29ca..f4b1147291e5 100644 > --- a/include/linux/iio/buffer_impl.h > +++ b/include/linux/iio/buffer_impl.h > @@ -9,8 +9,12 @@ > #include <uapi/linux/iio/buffer.h> > #include <linux/iio/buffer.h> > > +struct dma_buf_attachment; > +struct dma_fence; > struct iio_dev; > +struct iio_dma_buffer_block; > struct iio_buffer; > +struct sg_table; > > /** > * INDIO_BUFFER_FLAG_FIXED_WATERMARK - Watermark level of the buffer can not be > @@ -39,6 +43,13 @@ struct iio_buffer; > * device stops sampling. Calles are balanced with @enable. > * @release: called when the last reference to the buffer is dropped, > * should free all resources allocated by the buffer. > + * @attach_dmabuf: called from userspace via ioctl to attach one external > + * DMABUF. > + * @detach_dmabuf: called from userspace via ioctl to detach one previously > + * attached DMABUF. > + * @enqueue_dmabuf: called from userspace via ioctl to queue this DMABUF > + * object to this buffer. Requires a valid DMABUF fd, that > + * was previouly attached to this buffer. > * @modes: Supported operating modes by this buffer type > * @flags: A bitmask combination of INDIO_BUFFER_FLAG_* > * > @@ -68,6 +79,17 @@ struct iio_buffer_access_funcs { > > void (*release)(struct iio_buffer *buffer); > > + struct iio_dma_buffer_block * (*attach_dmabuf)(struct iio_buffer *buffer, > + struct dma_buf_attachment *attach); > + void (*detach_dmabuf)(struct iio_buffer *buffer, > + struct iio_dma_buffer_block *block); > + int (*enqueue_dmabuf)(struct iio_buffer *buffer, > + struct iio_dma_buffer_block *block, > + struct dma_fence *fence, struct sg_table *sgt, > + size_t size, bool cyclic); > + void (*lock_queue)(struct iio_buffer *buffer); > + void (*unlock_queue)(struct iio_buffer *buffer); > + > unsigned int modes; > unsigned int flags; > }; > @@ -136,6 +158,12 @@ struct iio_buffer { > > /* @ref: Reference count of the buffer. */ > struct kref ref; > + > + /* @dmabufs: List of DMABUF attachments */ > + struct list_head dmabufs; /* P: dmabufs_mutex */ > + > + /* @dmabufs_mutex: Protects dmabufs */ > + struct mutex dmabufs_mutex; > }; > > /** > @@ -156,9 +184,14 @@ int iio_update_buffers(struct iio_dev *indio_dev, > **/ > void iio_buffer_init(struct iio_buffer *buffer); > > +void iio_buffer_dmabuf_get(struct dma_buf_attachment *attach); > +void iio_buffer_dmabuf_put(struct dma_buf_attachment *attach); > + > struct iio_buffer *iio_buffer_get(struct iio_buffer *buffer); > void iio_buffer_put(struct iio_buffer *buffer); > > +void iio_buffer_signal_dmabuf_done(struct dma_fence *fence, int ret); > + > #else /* CONFIG_IIO_BUFFER */ > > static inline void iio_buffer_get(struct iio_buffer *buffer) {} > diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h > index 13939032b3f6..c666aa95e532 100644 > --- a/include/uapi/linux/iio/buffer.h > +++ b/include/uapi/linux/iio/buffer.h > @@ -5,6 +5,28 @@ > #ifndef _UAPI_IIO_BUFFER_H_ > #define _UAPI_IIO_BUFFER_H_ > > +#include <linux/types.h> > + > +/* Flags for iio_dmabuf.flags */ > +#define IIO_BUFFER_DMABUF_CYCLIC (1 << 0) > +#define IIO_BUFFER_DMABUF_SUPPORTED_FLAGS 0x00000001 > + > +/** > + * struct iio_dmabuf - Descriptor for a single IIO DMABUF object > + * @fd: file descriptor of the DMABUF object > + * @flags: one or more IIO_BUFFER_DMABUF_* flags > + * @bytes_used: number of bytes used in this DMABUF for the data transfer. > + * Should generally be set to the DMABUF's size. > + */ > +struct iio_dmabuf { > + __u32 fd; > + __u32 flags; > + __u64 bytes_used; > +}; > + > #define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int) > +#define IIO_BUFFER_DMABUF_ATTACH_IOCTL _IOW('i', 0x92, int) > +#define IIO_BUFFER_DMABUF_DETACH_IOCTL _IOW('i', 0x93, int) > +#define IIO_BUFFER_DMABUF_ENQUEUE_IOCTL _IOW('i', 0x94, struct iio_dmabuf) > > #endif /* _UAPI_IIO_BUFFER_H_ */ >

1 year, 9 months

3
8
0 0

Re: [PATCH v7 1/6] dmaengine: Add API function dmaengine_prep_peripheral_dma_vec()

by Nuno Sá

On Fri, 2024-02-23 at 13:13 +0100, Nuno Sa wrote: > From: Paul Cercueil <paul(a)crapouillou.net> > > This function can be used to initiate a scatter-gather DMA transfer, > where the address and size of each segment is located in one entry of > the dma_vec array. > > The major difference with dmaengine_prep_slave_sg() is that it supports > specifying the lengths of each DMA transfer; as trying to override the > length of the transfer with dmaengine_prep_slave_sg() is a very tedious > process. The introduction of a new API function is also justified by the > fact that scatterlists are on their way out. > > Note that dmaengine_prep_interleaved_dma() is not helpful either in that > case, as it assumes that the address of each segment will be higher than > the one of the previous segment, which we just cannot guarantee in case > of a scatter-gather transfer. > > Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> > Signed-off-by: Nuno Sa <nuno.sa(a)analog.com> > --- Hi Vinod, Is this already good for you? I do not want to be pushy but we're trying to see if we can have this in the 6.9 cycle and Jonathan definitely wants an ack from you before merging this in his tree. I've more or less till Wednesday so that's why I'm asking already today so I still have time to re-spin if you want some changes. - Nuno Sá

1 year, 9 months

1
0
0 0

Re: [PATCH v7 0/6] iio: new DMABUF based API

by Jonathan Cameron

On Fri, 23 Feb 2024 13:13:58 +0100 Nuno Sa <nuno.sa(a)analog.com> wrote: > Hi Jonathan, likely you're wondering why I'm sending v7. Well, to be > honest, we're hoping to get this merged this for the 6.9 merge window. > Main reason is because the USB part is already in (so it would be nice > to get the whole thing in). Moreover, the changes asked in v6 were simple > (even though I'm not quite sure in one of them) and Paul has no access to > it's laptop so he can't send v7 himself. So he kind of said/asked for me to do it. So, we are cutting this very fine. If Linus hints strongly at an rc8 maybe we can sneak this in. However, I need an Ack from Vinod for the dma engine changes first. Also I'd love a final 'looks ok' comment from DMABUF folk (Ack even better!) Seems that the other side got resolved in the USB gadget, but last we heard form Daniel and Christian looks to have been back on v5. I'd like them to confirm they are fine with the changes made as a result. I've been happy with the IIO parts for a few versions now but my ability to review the DMABUF and DMA engine bits is limited. A realistic path to get this in is rc8 is happening, is all Acks in place by Wednesday, I get apply it and hits Linux-next Thursday, Pull request to Greg on Saturday and Greg is feeling particularly generous to take one on the day he normally closes his trees. Whilst I'll cross my fingers, looks like 6.10 material to me :( I'd missed the progress on the USB side so wasn't paying enough attention. Sorry! Jonathan > > v6: > * https://lore.kernel.org/linux-iio/20240129170201.133785-1-paul@crapouillou.… > > v7: > - Patch 1 > * Renamed *device_prep_slave_dma_vec() -> device_prep_peripheral_dma_vec(); > * Added a new flag parameter to the function as agreed between Paul > and Vinod. I renamed the first parameter to prep_flags as it's supposed to > be used (I think) with enum dma_ctrl_flags. I'm not really sure how that API > can grow but I was thinking in just having a bool cyclic parameter (as the > first intention of the flags is to support cyclic transfers) but ended up > "respecting" the previously agreed approach. > - Patch 2 > * Adapted patch for the changes made in patch 1. > - Patch 5 > * Adapted patch for the changes made in patch 1. > > Patchset based on next-20240223. > > --- > Paul Cercueil (6): > dmaengine: Add API function dmaengine_prep_peripheral_dma_vec() > dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec > iio: core: Add new DMABUF interface infrastructure > iio: buffer-dma: Enable support for DMABUFs > iio: buffer-dmaengine: Support new DMABUF based userspace API > Documentation: iio: Document high-speed DMABUF based API > > Documentation/iio/dmabuf_api.rst | 54 +++ > Documentation/iio/index.rst | 2 + > drivers/dma/dma-axi-dmac.c | 40 ++ > drivers/iio/buffer/industrialio-buffer-dma.c | 181 +++++++- > drivers/iio/buffer/industrialio-buffer-dmaengine.c | 59 ++- > drivers/iio/industrialio-buffer.c | 462 +++++++++++++++++++++ > include/linux/dmaengine.h | 27 ++ > include/linux/iio/buffer-dma.h | 31 ++ > include/linux/iio/buffer_impl.h | 33 ++ > include/uapi/linux/iio/buffer.h | 22 + > 10 files changed, 894 insertions(+), 17 deletions(-) > --- > base-commit: 33e1d31873f87d119e5120b88cd350efa68ef276 > change-id: 20240223-iio-dmabuf-5ee0530195ca > -- > > Thanks! > - Nuno Sá >

1 year, 9 months

2
1
0 0

Re: [PATCH v2 2/2] net: ethernet: ti: am65-cpsw: Add minimal XDP support

by Andrew Lunn

On Fri, Mar 01, 2024 at 04:02:53PM +0100, Julien Panis wrote: > This patch adds XDP (eXpress Data Path) support to TI AM65 CPSW > Ethernet driver. The following features are implemented: > - NETDEV_XDP_ACT_BASIC (XDP_PASS, XDP_TX, XDP_DROP, XDP_ABORTED) > - NETDEV_XDP_ACT_REDIRECT (XDP_REDIRECT) > - NETDEV_XDP_ACT_NDO_XMIT (ndo_xdp_xmit callback) > > The page pool memory model is used to get better performance. Do you have any benchmark numbers? It should help with none XDP traffic as well. So maybe iperf numbers before and after? Andrew

1 year, 9 months

1
0
0 0

Re: (subset) [PATCH 13/18] mfd: mt6397-core: register mt6357 sound codec

by Lee Jones

On Mon, 26 Feb 2024 15:01:51 +0100, amergnat(a)baylibre.com wrote: > Add MT6357 codec entry in the MFD driver. > > Applied, thanks! [13/18] mfd: mt6397-core: register mt6357 sound codec commit: 79d98102a31ab777b4a6632d799ab2bc63654cf8 -- Lee Jones [李琼斯]

1 year, 9 months

1
0
0 0

[PATCH v2 0/3] Add GAMMA 12-bit LUT support for MT8188

by Jason-JH.Lin

From: Jason-jh Lin <jason-jh.lin(a)mediatek.corp-partner.google.com> Since MT8195 supports GAMMA 12-bit LUT after the landing of [1] series, we can now add support for MT8188. [1] MediaTek DDP GAMMA - 12-bit LUT support - https://patchwork.kernel.org/project/linux-mediatek/list/?series=792516 Change in v2: 1. Keep MT8195 compatible in the group of MT8183. 2. Move MT8195 compatible group to the end of items list. Jason-JH.Lin (3): dt-bindings: display: mediatek: gamma: Change MT8195 to single enum group dt-bindings: display: mediatek: gamma: Add support for MT8188 drm/mediatek: Add gamma support for MT8195 .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml | 5 +++++ drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 ++ 2 files changed, 7 insertions(+) -- 2.18.0

1 year, 9 months

3
5
0 0

Re: [PATCH] net: ethernet: ti: am65-cpsw: Add minimal XDP support

by Andrew Lunn

On Thu, Feb 29, 2024 at 05:19:44PM +0100, Julien Panis wrote: > On 2/27/24 00:18, Andrew Lunn wrote: > > > +static struct sk_buff *am65_cpsw_alloc_skb(struct net_device *ndev, unsigned int len) > > > +{ > > > + struct page *page; > > > + struct sk_buff *skb; > > > + > > > + page = dev_alloc_pages(0); > > You are likely to get better performance if you use the page_pool. > > > > When FEC added XDP support, the first set of changes was to make use > > of page_pool. That improved the drivers performance. Then XDP was > > added on top. Maybe you can follow that pattern. > > > > Andrew > > Hello Andrew, > > Thanks for this suggestion. I've been working on it over the last days. > I encountered issues and I begin to wonder if that's a good option for > this driver. Let me explain... I'm not a page pool expert, so hopefully those that are will jump in and help. > This device has 3 ports: > - Port0 is the host port (internal to the subsystem and referred as CPPI > in the driver). > - Port1 and 2 are the external ethernet ports. > Each port's RX FIFO has 1 queue. > > As mentioned in page_pool documentation: > https://docs.kernel.org/networking/page_pool.html > "The number of pools created MUST match the number of hardware > queues unless hardware restrictions make that impossible. This would > otherwise beat the purpose of page pool, which is allocate pages fast > from cache without locking." My guess is, this last bit is the important part. Locking. Do ports 1 and port 2 rx and tx run in parallel on different CPUs? Hence do you need locking? > So, for this driver I should use 2 page pools (for port1 and 2) if possible. Maybe, maybe not. It is not really the number of front panel interface which matters here. It is the number of entities which need buffers. > But, if I I replace any alloc_skb() with page_pool_alloc() in the original > driver, I will have to create only 1 page_pool. > This is visible in am65_cpsw_nuss_common_open(), which starts with: > "if (common->usage_count) return 0;" to ensure that subsequent code > will be executed only once even if the 2 interfaces are ndo_open'd. > IOW, skbs will be allocated for only 1 RX channel. I checked that behavior, > and that's the way it works. > This is because the host port (CPPI) has only 1 RX queue. This single > queue is used to get all the packets, from both Ports and 2 (port ID is > stored in each descriptor). So you have one entity which needs buffers. CPPI. It seems like Port1 and Port2 do not need buffers? So to me, you need one page pool. > So, because of this constraint, I ended up working on the "single > page pool" option. > > Questions: > 1) Is the behavior described above usual for ethernet switch devices ? This might be the first time page pool has been used by a switch? I would check the melanox and sparx5 driver and see if they use page pool. > 2) Given that I can't use a page pool for each HW queue, is it worth > using the page pool memory model ? > 3) Currently I use 2 xdp_rxq (one for port1 and another one for port2). > If an XDP program is attached to port1, my page pool dma parameter > will need to be DMA_BIDIRECTIONAL (because of XDP_TX). As a result, > even if there is no XDP program attached to port2, the setting for page > pool will need to be DMA_BIDIRECTIONAL instead of DMA_FROM_DEVICE. > In such situation, isn't it a problem for port2 ? > 4) Because of 1) and 2), will page pool performance really be better for > this driver ? You probably need to explain the TX architecture a bit. How are packets passed to the hardware? Is it again via a single CPPI entity? Or are there two entities? DMA_BIDIRECTIONAL and DMA_FROM_DEVICE is about cache flushing and invalidation. Before you pass a buffer to the hardware for it to receive into, you need to invalidate the cache. That means when the hardware gives the buffer back with a packet in it, there is a cache miss and it fetches the new data from memory. If that packet gets turned into an XDP_TX, you need to flush the cache to force any changes out of the cache and into memory. The DMA from memory then gets the up to date packet contents. My guess would be, always using DMA_BIDIRECTIONAL is fine, so long as your cache operations are correct. Turn on DMA_API_DEBUG and make sure it is happy. Andrew

1 year, 9 months

1
0
0 0

[PATCH 0/3] Add GAMMA 12-bit LUT support for MT8188

by Jason-JH.Lin

From: Jason-jh Lin <jason-jh.lin(a)mediatek.corp-partner.google.com> Since MT8195 supports GAMMA 12-bit LUT after the landing of [1] series, we can now add support for MT8188. [1] MediaTek DDP GAMMA - 12-bit LUT support - https://patchwork.kernel.org/project/linux-mediatek/list/?series=792516 Jason-JH.Lin (3): dt-bindings: display: mediatek: gamma: Change MT8195 to single enum group dt-bindings: display: mediatek: gamma: Add support for MT8188 drm/mediatek: Add gamma support for MT8195 .../bindings/display/mediatek/mediatek,gamma.yaml | 6 +++++- drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) -- 2.18.0

1 year, 9 months

3
8
0 0

Re: ECC memory semantics for heaps

by John Stultz

On Wed, Feb 28, 2024 at 7:24 AM Maxime Ripard <mripard(a)redhat.com> wrote: > > I'm currently working on a platform that seems to have togglable RAM ECC > support. Enabling ECC reduces the memory capacity and memory bandwidth, > so while it's a good idea to protect most of the system, it's not worth > it for things like framebuffers that won't really be affected by a > bitflip. > > It's currently setup by enabling ECC on the entire memory, and then > having a region of memory where ECC is disabled and where we're supposed > to allocate from for allocations that don't need it. > > My first thought to support this was to create a reserved memory region > for the !ECC memory, and to create a heap to allocate buffers from that > region. That would leave the system protected by ECC, while enabling > userspace to be nicer to the system by allocating buffers from the !ECC > region if it doesn't need it. > > However, this creates basically a new combination compared to the one we > already have (ie, physically contiguous vs virtually contiguous), and we > probably would want to throw in cacheable vs non-cacheable too. > > If we had to provide new heaps for each variation, we would have 8 heaps > (and 6 new ones), which could be fine I guess but would still increase > quite a lot the number of heaps we have so far. > > Is it something that would be a problem? If it is, do you see another > way to support those kind of allocations (like providing hints through > the ioctl maybe?)? So, the dma-buf heaps interface uses chardevs so that we can have a lot of flexibility in the types of heaps (and don't have the risk of bitmask exhaustion like ION had). So I don't see adding many differently named heaps as particularly problematic. That said, if there are truly generic properties (cacheable vs non-cachable is maybe one of those) which apply to most heaps, I'm open to making use of the flags. But I want to avoid having per-heap flags, it really needs to be a generic attribute. And I personally don't mind the idea of having things added as heaps initially, and potentially upgrading them to flags if needed (allowing heap drivers to optionally enumerate the old chardevs behind a config option for backwards compatibility). How common is the hardware that is going to provide this configurable ECC option, and will you really want the option on all of the heap types? Will there be any hardware constraint limitations caused by the ECC/!ECC flags? (ie: Devices that can't use !ECC allocated buffers?) If not, I wonder if it would make sense to have something more along the lines using a fcntl() like how F_SEAL_* is used with memfds? With some of the discussion around "restricted"/"secure" heaps that can change state, I've liked this idea of just allocating dmabufs from normal heaps and then using fcntl or something similar to modify properties of the buffer that are separate from the type of memory that is needed to be allocated to satisfy device constraints. thanks -john

1 year, 9 months

1
0
0 0

Re: [PATCH 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document

by AngeloGioacchino Del Regno

Il 28/02/24 10:57, Alexandre Mergnat ha scritto: > I think I got it. > > - mediatek,i2s-shared-clock: will be remove from DT > - mediatek,dmic-iir-on: will be remove from DT > - mediatek,dmic-irr-mode: will be remove from DT > - mediatek,dmic-two-wire-mode: rephrase description needed > > I've did abstraction (despite me) that IIR settings are runtime config because the > driver implement its usage like a one-time-setup -_-' > Yes but just one more thing I just noticed: `mediatek,dmic-two-wire-mode` - can we please rename this to `mediatek,dmic-mode` ? That'd be for consistency check mt6359.yaml and mt6358.txt mediatek,dmic-mode: $ref: /schemas/types.yaml#/definitions/uint32 description: | Indicates how many data pins are used to transmit two channels of PDM signal. 0 means two wires, 1 means one wire. Default value is 0. enum: - 0 # one wire - 1 # two wires Cheers, Angelo > Thanks for the explanations, that help. > > Regards, > Alexandre > > On 28/02/2024 08:28, Krzysztof Kozlowski wrote: >> On 27/02/2024 16:18, Alexandre Mergnat wrote: >>>> >>>>> + type: boolean >>>>> + >>>>> + mediatek,dmic-iir-on: >>>>> + description: >>>>> + Boolean which specifies whether the DMIC IIR is enabled. >>>>> + If this property is not present the IIR is disabled. >>>> >>>> "is enabled" or "enable it"? >>>> >>>> You described the desired Linux feature or behavior, not the actual >>>> hardware. The bindings are about the latter, so instead you need to >>>> rephrase the property and its description to match actual hardware >>>> capabilities/features/configuration etc. >>> >>> I will rephrase: >>> >>> True to enable the Infinite Impulse Response (IIR) filter >>> on the digital microphone inputs. >> >> I still don't know why this is DT-specific. You still tell driver what >> to do... >> >>> >>>> >>>>> + type: boolean >>>>> + >>>>> + mediatek,dmic-irr-mode: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + description: >>>>> + Selects stop band of IIR DC-removal filter. >>>>> + 0 = Software programmable custom coeff loaded by the driver. >>>> >>>> Bindings are for hardware, not drivers. Why is this a property of board DTS? >>> >>> Actually this is a hardware feature. Mode 1 t 5 are predefined filters. >>> Mode 0, the HW will read some "coef filter registers" to setup the >>> custom filter. the "coef filter regs" are written by the driver. >>> Currently the coef values are hardcoded in the driver. >> >> You don't get the point. Just because you choose some mode it does not >> mean is hardware feature for DT. Sampling frequency done by hardware is >> also "hardware feature", but do you put it to DT? No. >> >> Explain why this is board-specific, not runtime configuration. >> >>> >>>> >>>>> + 1 = 5Hz if 48KHz mode. >>>>> + 2 = 10Hz if 48KHz mode. >>>>> + 3 = 25Hz if 48KHz mode. >>>>> + 4 = 50Hz if 48KHz mode. >>>>> + 5 = 65Hz if 48KHz mode. >>>> >>>> Use proper unit suffixes - hz. >>>> >>>> >>>>> + enum: >>>>> + - 0 >>>>> + - 1 >>>>> + - 2 >>>>> + - 3 >>>>> + - 4 >>>>> + - 5 >>>>> + >>>>> + mediatek,dmic-two-wire-mode: >>>>> + description: >>>>> + Boolean which turns on digital microphone for two wire mode. >>>>> + If this property is not present the two wire mode is disabled. >>>> >>>> This looks like hardware property, but the naming looks like SW. Again >>>> you instruct what driver should do. Standard disclaimer: >>>> >>>> You described the desired Linux feature or behavior, not the actual >>>> hardware. The bindings are about the latter, so instead you need to >>>> rephrase the property and its description to match actual hardware >>>> capabilities/features/configuration etc. >>> >>> Actually this is a hardware feature. This is ALL I have to describe the >>> HW behavior from the datasheet: >>> " >>> bit name: ul_dmic_two_wire_ctl >>> Turns on digital microphone for two wire mode. >>> 0: Turn off >>> 1: Turn on >> >> That's rather suggestion it is not a description of hardware but you >> want driver to do something... >> >>> " >>> >>> On the board schematic, SoC and DMIC and linked by 3 pins: >>> - clk >>> - data0 >>> - data1 >>> >>> IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must >>> be aware of that by reading the register value written by the driver, >>> using the value found in the DTS. >> >> So this depends on type of connection of DMIC? Then rephrase description >> property like this. >> >>> >>> I don't get why you think it wouldn't be hardware behavior. >> >> Because telling what to write to the registers which is typical sign of >> people stuffing to DT whatever they need to configure the hardware. >> >>> >>> Rephrase description: >>> "True to enable the two wire mode of the digital microphone" >>> Is it better ? >> >> No, because again you describe some sort of mode. If you insist on such >> description, then my answer is: it's runtime, so not suitable for DT. >> Instead describe what is the hardware problem/configuration, e.g. "DMIC >> is connected with only CLK and DATA0, without third pin" etc. >> >>> >>> About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ? >> >> To sound more like a register less like physical characteristic of the >> board? No. The name can stay, I don't have better ideas. >> >> >> Best regards, >> Krzysztof >> >

1 year, 9 months

2
1
0 0

Re: [PATCH 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document

by Krzysztof Kozlowski

On 27/02/2024 16:18, Alexandre Mergnat wrote: >> >>> + type: boolean >>> + >>> + mediatek,dmic-iir-on: >>> + description: >>> + Boolean which specifies whether the DMIC IIR is enabled. >>> + If this property is not present the IIR is disabled. >> >> "is enabled" or "enable it"? >> >> You described the desired Linux feature or behavior, not the actual >> hardware. The bindings are about the latter, so instead you need to >> rephrase the property and its description to match actual hardware >> capabilities/features/configuration etc. > > I will rephrase: > > True to enable the Infinite Impulse Response (IIR) filter > on the digital microphone inputs. I still don't know why this is DT-specific. You still tell driver what to do... > >> >>> + type: boolean >>> + >>> + mediatek,dmic-irr-mode: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Selects stop band of IIR DC-removal filter. >>> + 0 = Software programmable custom coeff loaded by the driver. >> >> Bindings are for hardware, not drivers. Why is this a property of board DTS? > > Actually this is a hardware feature. Mode 1 t 5 are predefined filters. > Mode 0, the HW will read some "coef filter registers" to setup the > custom filter. the "coef filter regs" are written by the driver. > Currently the coef values are hardcoded in the driver. You don't get the point. Just because you choose some mode it does not mean is hardware feature for DT. Sampling frequency done by hardware is also "hardware feature", but do you put it to DT? No. Explain why this is board-specific, not runtime configuration. > >> >>> + 1 = 5Hz if 48KHz mode. >>> + 2 = 10Hz if 48KHz mode. >>> + 3 = 25Hz if 48KHz mode. >>> + 4 = 50Hz if 48KHz mode. >>> + 5 = 65Hz if 48KHz mode. >> >> Use proper unit suffixes - hz. >> >> >>> + enum: >>> + - 0 >>> + - 1 >>> + - 2 >>> + - 3 >>> + - 4 >>> + - 5 >>> + >>> + mediatek,dmic-two-wire-mode: >>> + description: >>> + Boolean which turns on digital microphone for two wire mode. >>> + If this property is not present the two wire mode is disabled. >> >> This looks like hardware property, but the naming looks like SW. Again >> you instruct what driver should do. Standard disclaimer: >> >> You described the desired Linux feature or behavior, not the actual >> hardware. The bindings are about the latter, so instead you need to >> rephrase the property and its description to match actual hardware >> capabilities/features/configuration etc. > > Actually this is a hardware feature. This is ALL I have to describe the > HW behavior from the datasheet: > " > bit name: ul_dmic_two_wire_ctl > Turns on digital microphone for two wire mode. > 0: Turn off > 1: Turn on That's rather suggestion it is not a description of hardware but you want driver to do something... > " > > On the board schematic, SoC and DMIC and linked by 3 pins: > - clk > - data0 > - data1 > > IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must > be aware of that by reading the register value written by the driver, > using the value found in the DTS. So this depends on type of connection of DMIC? Then rephrase description property like this. > > I don't get why you think it wouldn't be hardware behavior. Because telling what to write to the registers which is typical sign of people stuffing to DT whatever they need to configure the hardware. > > Rephrase description: > "True to enable the two wire mode of the digital microphone" > Is it better ? No, because again you describe some sort of mode. If you insist on such description, then my answer is: it's runtime, so not suitable for DT. Instead describe what is the hardware problem/configuration, e.g. "DMIC is connected with only CLK and DATA0, without third pin" etc. > > About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ? To sound more like a register less like physical characteristic of the board? No. The name can stay, I don't have better ideas. Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 00/18] Add audio support for the MediaTek Genio 350-evk board

by Mark Brown

On Mon, Feb 26, 2024 at 03:01:38PM +0100, Alexandre Mergnat wrote: > This serie aim to add the following audio support for the Genio 350-evk: > - Playback > - 2ch Headset Jack (Earphone) > - 1ch Line-out Jack (Speaker) > - 8ch HDMI Tx > - Capture > - 1ch DMIC (On-board Digital Microphone) > - 1ch AMIC (On-board Analogic Microphone) > - 1ch Headset Jack (External Analogic Microphone) > > Of course, HDMI playback need the MT8365 display patches [1] and a DTS > change documented in "mediatek,mt8365-mt6357.yaml". Given the number of custom controls here could you please post the output of mixer-test and pcm-test from a system with this driver running next time you post, this will help with review since it checks a bunch of things around the new controls.

1 year, 9 months

1
0
0 0

Re: [PATCH 02/18] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document

by AngeloGioacchino Del Regno

Il 27/02/24 11:23, Alexandre Mergnat ha scritto: > > > On 26/02/2024 16:30, AngeloGioacchino Del Regno wrote: >> Il 26/02/24 15:01, Alexandre Mergnat ha scritto: >>> Add soundcard bindings for the MT8365 SoC with the MT6357 audio codec. >>> >>> Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> >>> --- >>> .../bindings/sound/mediatek,mt8365-mt6357.yaml | 127 +++++++++++++++++++++ >>> 1 file changed, 127 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml >>> b/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml >>> new file mode 100644 >>> index 000000000000..f469611ec6b6 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml >>> @@ -0,0 +1,127 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-mt6357.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Mediatek MT8365 sound card with MT6357 sound codec. >>> + >>> +maintainers: >>> + - Alexandre Mergnat <amergnat(a)baylibre.com> >>> + >>> +description: >>> + This binding describes the MT8365 sound card. >>> + >>> +properties: >>> + compatible: >>> + const: mediatek,mt8365-mt6357 >>> + >>> + mediatek,hp-pull-down: >>> + description: >>> + Earphone driver positive output stage short to the >>> + audio reference ground. >>> + Default value is false. >>> + type: boolean >>> + >>> + mediatek,micbias0-microvolt: >>> + description: | >> >> description: Selects MIC Bias 0 output voltage >> >>> + Selects MIC Bias 0 output voltage. >>> + [1.7v, 1.8v, 1.9v, 2.0v, 2.1v, 2.5v, 2.6v, 2.7v] >>> + enum: [0, 1, 2, 3, 4, 5, 6, 7] >> >> No, you don't say 0 1 2 3 4 to a property that says "microvolt", that's simply >> wrong. >> >> mediatek,micbias0-microvolt = <2100000>; >> >> ...so you want a binding that says >> enum: [ 1700000, 1800000, this, that, 2700000] >> > > Is it correct if I put "description: Selects MIC Bias 0 output voltage index" ? > No, it's still not correct. You have to pass microvolt values. The driver will then transform the microvolt values to the index and subsequently write the index value to the hardware registers. The bindings shall be generic, in the sense that they shall not express hardware register values... and this is especially true when we have a value that *does* actually have means to be expressed in common units. Besides, in the cases in which there's no common units involved, the values most probably won't be suited for devicetree//bindings, so those would be hardcoded in the driver as platform data. This is not the case, so, please keep this property but specify microvolts in the bindings (and obviously in devicetree). >>> + >>> + mediatek,micbias1-microvolt: >>> + description: | >>> + Selects MIC Bias 1 output voltage. >>> + [1.7v, 1.8v, 1.9v, 2.0v, 2.1v, 2.5v, 2.6v, 2.7v] >>> + enum: [0, 1, 2, 3, 4, 5, 6, 7] >> >> same here. >> >>> + >>> + mediatek,platform: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: The phandle of MT8365 ASoC platform. >>> + >>> + pinctrl-names: >>> + minItems: 1 >>> + items: >>> + - const: aud_default >>> + - const: aud_dmic >>> + - const: aud_miso_off >>> + - const: aud_miso_on >>> + - const: aud_mosi_off >>> + - const: aud_mosi_on >>> + >>> + vaud28-supply: >>> + description: >>> + 2.8 volt supply for the audio codec >>> + >>> +patternProperties: >>> + "^dai-link-[0-9]+$": >>> + type: object >>> + description: >>> + Container for dai-link level properties and CODEC sub-nodes. >>> + >>> + properties: >>> + codec: >>> + type: object >>> + description: Holds subnode which indicates codec dai. >>> + >>> + properties: >>> + sound-dai: >>> + maxItems: 1 >>> + description: phandle of the codec DAI >>> + >>> + additionalProperties: false >>> + >>> + link-name: >>> + description: >>> + This property corresponds to the name of the BE dai-link to which >>> + we are going to update parameters in this node. >>> + items: >>> + const: 2ND I2S BE >>> + >>> + sound-dai: >>> + maxItems: 1 >>> + description: phandle of the CPU DAI >>> + >>> + additionalProperties: false >>> + >>> + required: >>> + - link-name >>> + - sound-dai >>> + >>> +additionalProperties: false >>> + >>> +required: >>> + - compatible >>> + - mediatek,platform >>> + - pinctrl-names >>> + - vaud28-supply >>> + >>> +examples: >>> + - | >>> + sound { >>> + compatible = "mediatek,mt8365-mt6357"; >>> + mediatek,platform = <&afe>; >> >> Please: >> >> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html > > Is it about the wrong pinctrl-names tab alignment ? > Also, 2ND I2S BE => 2ND_I2S_BE ? > Otherwise, I don't get it sorry. > ...as Krzysztof already clarified, won't repeat :-P Cheers! >> >> Regards, >> Angelo >> >>> + pinctrl-names = "aud_default", >>> + "aud_dmic", >>> + "aud_miso_off", >>> + "aud_miso_on", >>> + "aud_mosi_off", >>> + "aud_mosi_on"; >>> + pinctrl-0 = <&aud_default_pins>; >>> + pinctrl-1 = <&aud_dmic_pins>; >>> + pinctrl-2 = <&aud_miso_off_pins>; >>> + pinctrl-3 = <&aud_miso_on_pins>; >>> + pinctrl-4 = <&aud_mosi_off_pins>; >>> + pinctrl-5 = <&aud_mosi_on_pins>; >>> + vaud28-supply = <&mt6357_vaud28_reg>; >>> + >>> + /* hdmi interface */ >>> + dai-link-0 { >>> + sound-dai = <&afe>; >>> + link-name = "2ND I2S BE"; >>> + >>> + codec { >>> + sound-dai = <&it66121hdmitx>; >>> + }; >>> + }; >>> + }; >>> >> >

1 year, 9 months

1
0
0 0

Re: [PATCH 02/18] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document

by Krzysztof Kozlowski

On 27/02/2024 11:23, Alexandre Mergnat wrote: >>> + >>> +examples: >>> + - | >>> + sound { >>> + compatible = "mediatek,mt8365-mt6357"; >>> + mediatek,platform = <&afe>; >> >> Please: >> >> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html > > Is it about the wrong pinctrl-names tab alignment ? > Also, 2ND I2S BE => 2ND_I2S_BE ? > Otherwise, I don't get it sorry. Alignment of continued lines, order of properties. Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 08/18] ASoC: mediatek: mt8365: Add DMIC DAI support

by Krzysztof Kozlowski

On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add Digital Micro Device Audio Interface support for MT8365 SoC. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > + > +static int init_dmic_priv_data(struct mtk_base_afe *afe) > +{ > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + struct mt8365_dmic_data *dmic_priv; > + struct device_node *np = afe->dev->of_node; > + unsigned int temps[4]; > + int ret; > + > + dmic_priv = devm_kzalloc(afe->dev, sizeof(struct mt8365_dmic_data), > + GFP_KERNEL); You have very inconsistent style of coding. Some patches are done correctly, some repeast known issues. All over. This is sizeof(*). This comment (and all others) apply everywhere, just in case. Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 03/18] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add the codec property along with the mt6357.c codec driver support. Describe the hardware, not the Linux drivers. There is no codec driver support in the bindings. https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/b… > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > index 37423c2e0fdf..d25a78070744 100644 > --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > @@ -37,6 +37,17 @@ properties: > "#interrupt-cells": > const: 2 > > + codec: > + type: object > + unevaluatedProperties: false > + description: > + MT6357 sound codec. > + properties: > + compatible: > + const: mediatek,mt6357-sound > + required: > + - compatible No resources? Then no need for this node. We have it even documented (if my repeating every time is not enough)... https://elixir.bootlin.com/linux/v6.8-rc6/source/Documentation/devicetree/b… Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 02/18] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document

by Krzysztof Kozlowski

On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add soundcard bindings for the MT8365 SoC with the MT6357 audio codec. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > .../bindings/sound/mediatek,mt8365-mt6357.yaml | 127 +++++++++++++++++++++ > 1 file changed, 127 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml > new file mode 100644 > index 000000000000..f469611ec6b6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml > @@ -0,0 +1,127 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-mt6357.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek MT8365 sound card with MT6357 sound codec. Drop full stop. This is title. > + > +maintainers: > + - Alexandre Mergnat <amergnat(a)baylibre.com> > + > +description: > + This binding describes the MT8365 sound card. Say something useful. There is no need to say that binding is a binding describing something. Or just drop it if this is obvious. > + > +properties: > + compatible: > + const: mediatek,mt8365-mt6357 > + > + mediatek,hp-pull-down: > + description: > + Earphone driver positive output stage short to the > + audio reference ground. > + Default value is false. That's obvious, isn't it? Drop. > + type: boolean > + > + mediatek,micbias0-microvolt: > + description: | > + Selects MIC Bias 0 output voltage. > + [1.7v, 1.8v, 1.9v, 2.0v, 2.1v, 2.5v, 2.6v, 2.7v] > + enum: [0, 1, 2, 3, 4, 5, 6, 7] 0 is 0V, not 1.7! That's total mess. You use here microvolts. Look at property name. > + > + mediatek,micbias1-microvolt: > + description: | > + Selects MIC Bias 1 output voltage. > + [1.7v, 1.8v, 1.9v, 2.0v, 2.1v, 2.5v, 2.6v, 2.7v] > + enum: [0, 1, 2, 3, 4, 5, 6, 7] > + > + mediatek,platform: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of MT8365 ASoC platform. > + > + pinctrl-names: > + minItems: 1 > + items: > + - const: aud_default Drop redundant parts. aud looks like block name. > + - const: aud_dmic > + - const: aud_miso_off > + - const: aud_miso_on > + - const: aud_mosi_off > + - const: aud_mosi_on > + > + vaud28-supply: > + description: > + 2.8 volt supply for the audio codec > + > +patternProperties: > + "^dai-link-[0-9]+$": > + type: object > + description: > + Container for dai-link level properties and CODEC sub-nodes. > + > + properties: > + codec: > + type: object > + description: Holds subnode which indicates codec dai. > + > + properties: > + sound-dai: > + maxItems: 1 > + description: phandle of the codec DAI > + > + additionalProperties: false > + > + link-name: > + description: > + This property corresponds to the name of the BE dai-link to which > + we are going to update parameters in this node. > + items: > + const: 2ND I2S BE > + > + sound-dai: > + maxItems: 1 > + description: phandle of the CPU DAI > + > + additionalProperties: false > + > + required: > + - link-name > + - sound-dai > + > +additionalProperties: false This goes after required: block. See example schema. > + > +required: > + - compatible > + - mediatek,platform > + - pinctrl-names > + - vaud28-supply > + Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document

by Krzysztof Kozlowski

On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add MT8365 audio front-end bindings > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > .../bindings/sound/mediatek,mt8365-afe.yaml | 164 +++++++++++++++++++++ > 1 file changed, 164 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml > new file mode 100644 > index 000000000000..1d7eb2532ad2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml > @@ -0,0 +1,164 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-afe.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek AFE PCM controller for MT8365 > + > +maintainers: > + - Alexandre Mergnat <amergnat(a)baylibre.com> > + > +properties: > + compatible: > + const: mediatek,mt8365-afe-pcm > + > + reg: > + maxItems: 2 You must describe the items. > + > + interrupts: > + maxItems: 1 > + > + mediatek,topckgen: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of the mediatek topckgen controller What for? Don't repeat the property name. Say something useful. > + > + power-domains: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 0 > + > + clocks: > + items: > + - description: 26M clock > + - description: mux for audio clock > + - description: audio i2s0 mck > + - description: audio i2s1 mck > + - description: audio i2s2 mck > + - description: audio i2s3 mck > + - description: engen 1 clock > + - description: engen 2 clock > + - description: audio 1 clock > + - description: audio 2 clock > + - description: mux for i2s0 > + - description: mux for i2s1 > + - description: mux for i2s2 > + - description: mux for i2s3 > + > + clock-names: > + items: > + - const: top_clk26m_clk > + - const: top_audio_sel > + - const: audio_i2s0_m > + - const: audio_i2s1_m > + - const: audio_i2s2_m > + - const: audio_i2s3_m > + - const: engen1 > + - const: engen2 > + - const: aud1 > + - const: aud2 > + - const: i2s0_m_sel > + - const: i2s1_m_sel > + - const: i2s2_m_sel > + - const: i2s3_m_sel > + > + mediatek,i2s-shared-clock: Why do you need this property? Linux (and other systems) handle sharing clocks properly. > + description: > + i2s modules can share the same external clock pin. > + If this property is not present the clock mode is separrate. Typo > + type: boolean > + > + mediatek,dmic-iir-on: > + description: > + Boolean which specifies whether the DMIC IIR is enabled. > + If this property is not present the IIR is disabled. "is enabled" or "enable it"? You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + type: boolean > + > + mediatek,dmic-irr-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Selects stop band of IIR DC-removal filter. > + 0 = Software programmable custom coeff loaded by the driver. Bindings are for hardware, not drivers. Why is this a property of board DTS? > + 1 = 5Hz if 48KHz mode. > + 2 = 10Hz if 48KHz mode. > + 3 = 25Hz if 48KHz mode. > + 4 = 50Hz if 48KHz mode. > + 5 = 65Hz if 48KHz mode. Use proper unit suffixes - hz. > + enum: > + - 0 > + - 1 > + - 2 > + - 3 > + - 4 > + - 5 > + > + mediatek,dmic-two-wire-mode: > + description: > + Boolean which turns on digital microphone for two wire mode. > + If this property is not present the two wire mode is disabled. This looks like hardware property, but the naming looks like SW. Again you instruct what driver should do. Standard disclaimer: You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc. > + type: boolean > + > + Just one blank line. > +required: > + - compatible > + - reg > + - interrupts > + - power-domains > + - mediatek,topckgen > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/mediatek,mt8365-clk.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/power/mediatek,mt8365-power.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + afe@11220000 { What is AFE? https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetr… > + compatible = "mediatek,mt8365-afe-pcm"; Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 11/18] ASoC: mediatek: mt8365: Add platform driver

by Krzysztof Kozlowski

On 26/02/2024 15:01, Alexandre Mergnat wrote: > Add mt8365 platform driver. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> ... > + memif->substream = substream; > + > + snd_pcm_hw_constraint_step(substream->runtime, 0, > + SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 16); > + > + snd_soc_set_runtime_hwparams(substream, mtk_afe_hardware); > + > + ret = snd_pcm_hw_constraint_integer(runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) > + dev_err(afe->dev, "snd_pcm_hw_constraint_integer failed\n"); > + > + mt8365_afe_enable_main_clk(afe); > + return ret; > +} > + > +static void mt8365_afe_fe_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + int memif_num = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[memif_num]; > + > + dev_dbg(afe->dev, "%s %s\n", __func__, memif->data->name); Drop trace-like prints. Tracing is for this. Before upstreaming driver from vendor repository, such stuff should be cleaned. > + > + memif->substream = NULL; > + > + mt8365_afe_disable_main_clk(afe); > +} > + > +static int mt8365_afe_fe_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mt8365_control_data *ctrl_data = &afe_priv->ctrl_data; > + struct mtk_base_afe_memif *memif = &afe->memif[dai_id]; > + struct mt8365_fe_dai_data *fe_data = &afe_priv->fe_data[dai_id]; > + const size_t request_size = params_buffer_bytes(params); > + unsigned int channels = params_channels(params); > + unsigned int rate = params_rate(params); > + int ret, fs = 0; > + unsigned int base_end_offset = 8; > + > + dev_info(afe->dev, > + "%s %s period = %d rate = %d channels = %d\n", > + __func__, memif->data->name, params_period_size(params), > + rate, channels); > + > + if (dai_id == MT8365_AFE_MEMIF_VUL2) { > + if (!ctrl_data->bypass_cm1) > + /* configure cm1 */ > + mt8365_afe_configure_cm(afe, > + MT8365_CM1, channels, rate); > + else > + regmap_update_bits(afe->regmap, AFE_CM1_CON0, > + CM_AFE_CM1_VUL_SEL, CM_AFE_CM1_VUL_SEL); > + } else if (dai_id == MT8365_AFE_MEMIF_TDM_IN) { > + if (!ctrl_data->bypass_cm2) > + /* configure cm2 */ > + mt8365_afe_configure_cm(afe, > + MT8365_CM2, channels, rate); > + else > + regmap_update_bits(afe->regmap, AFE_CM2_CON0, > + CM_AFE_CM2_TDM_SEL, ~CM_AFE_CM2_TDM_SEL); > + > + base_end_offset = 4; > + } > + > + if (request_size > fe_data->sram_size) { > + ret = snd_pcm_lib_malloc_pages(substream, request_size); > + if (ret < 0) { > + dev_err(afe->dev, > + "%s %s malloc pages %zu bytes failed %d\n", > + __func__, memif->data->name, request_size, ret); > + return ret; > + } > + > + fe_data->use_sram = false; > + > + mt8365_afe_emi_clk_on(afe); > + } else { > + struct snd_dma_buffer *dma_buf = &substream->dma_buffer; > + > + dma_buf->dev.type = SNDRV_DMA_TYPE_DEV; > + dma_buf->dev.dev = substream->pcm->card->dev; > + dma_buf->area = (unsigned char *)fe_data->sram_vir_addr; > + dma_buf->addr = fe_data->sram_phy_addr; > + dma_buf->bytes = request_size; > + snd_pcm_set_runtime_buffer(substream, dma_buf); > + > + fe_data->use_sram = true; > + } > + > + memif->phys_buf_addr = lower_32_bits(substream->runtime->dma_addr); > + memif->buffer_size = substream->runtime->dma_bytes; > + > + /* start */ > + regmap_write(afe->regmap, memif->data->reg_ofs_base, > + memif->phys_buf_addr); > + /* end */ > + regmap_write(afe->regmap, > + memif->data->reg_ofs_base + base_end_offset, > + memif->phys_buf_addr + memif->buffer_size - 1); > + > + /* set channel */ > + if (memif->data->mono_shift >= 0) { > + unsigned int mono = (params_channels(params) == 1) ? 1 : 0; > + > + if (memif->data->mono_reg < 0) > + dev_info(afe->dev, "%s mono_reg is NULL\n", __func__); > + else > + regmap_update_bits(afe->regmap, memif->data->mono_reg, > + 1 << memif->data->mono_shift, > + mono << memif->data->mono_shift); > + } > + > + /* set rate */ > + if (memif->data->fs_shift < 0) > + return 0; > + > + fs = afe->memif_fs(substream, params_rate(params)); > + > + if (fs < 0) > + return -EINVAL; > + > + if (memif->data->fs_reg < 0) > + dev_info(afe->dev, "%s fs_reg is NULL\n", __func__); > + else > + regmap_update_bits(afe->regmap, memif->data->fs_reg, > + memif->data->fs_maskbit << memif->data->fs_shift, > + fs << memif->data->fs_shift); > + > + return 0; > +} > + > +static int mt8365_afe_fe_hw_free(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[dai_id]; > + struct mt8365_fe_dai_data *fe_data = &afe_priv->fe_data[dai_id]; > + int ret = 0; > + > + dev_dbg(afe->dev, "%s %s\n", __func__, memif->data->name); Drop > + > + if (fe_data->use_sram) { > + snd_pcm_set_runtime_buffer(substream, NULL); > + } else { > + ret = snd_pcm_lib_free_pages(substream); > + > + mt8365_afe_emi_clk_off(afe); > + } > + > + return ret; > +} > + > +static int mt8365_afe_fe_prepare(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mtk_base_afe_memif *memif = &afe->memif[dai_id]; > + > + /* set format */ > + if (memif->data->hd_reg >= 0) { > + switch (substream->runtime->format) { > + case SNDRV_PCM_FORMAT_S16_LE: > + regmap_update_bits(afe->regmap, memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 0 << memif->data->hd_shift); > + break; > + case SNDRV_PCM_FORMAT_S32_LE: > + regmap_update_bits(afe->regmap, memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 3 << memif->data->hd_shift); > + > + if (dai_id == MT8365_AFE_MEMIF_TDM_IN) { > + regmap_update_bits(afe->regmap, > + memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 1 << memif->data->hd_shift); > + regmap_update_bits(afe->regmap, > + memif->data->hd_reg, > + 1 << memif->data->hd_align_mshift, > + 1 << memif->data->hd_align_mshift); > + } > + break; > + case SNDRV_PCM_FORMAT_S24_LE: > + regmap_update_bits(afe->regmap, memif->data->hd_reg, > + 3 << memif->data->hd_shift, > + 1 << memif->data->hd_shift); > + break; > + default: > + return -EINVAL; > + } > + } > + > + mt8365_afe_irq_direction_enable(afe, > + memif->irq_usage, > + MT8365_AFE_IRQ_DIR_MCU); > + > + return 0; > +} > + > +int mt8365_afe_fe_trigger(struct snd_pcm_substream *substream, int cmd, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id; > + struct mt8365_control_data *ctrl_data = &afe_priv->ctrl_data; > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + /* enable channel merge */ > + if (dai_id == MT8365_AFE_MEMIF_VUL2 && > + !ctrl_data->bypass_cm1) { > + regmap_update_bits(afe->regmap, > + AFE_CM1_CON0, > + CM_AFE_CM_ON, CM_AFE_CM_ON); > + } else if (dai_id == MT8365_AFE_MEMIF_TDM_IN && > + !ctrl_data->bypass_cm2) { > + regmap_update_bits(afe->regmap, > + AFE_CM2_CON0, > + CM_AFE_CM_ON, CM_AFE_CM_ON); > + } > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + case SNDRV_PCM_TRIGGER_SUSPEND: > + /* disable channel merge */ > + if (dai_id == MT8365_AFE_MEMIF_VUL2 && > + !ctrl_data->bypass_cm1) { > + regmap_update_bits(afe->regmap, > + AFE_CM1_CON0, > + CM_AFE_CM_ON, ~CM_AFE_CM_ON); > + } else if (dai_id == MT8365_AFE_MEMIF_TDM_IN && > + !ctrl_data->bypass_cm2) { > + regmap_update_bits(afe->regmap, > + AFE_CM2_CON0, > + CM_AFE_CM_ON, ~CM_AFE_CM_ON); > + } > + break; > + default: > + break; > + } > + > + return mtk_afe_fe_trigger(substream, cmd, dai); > +} > + > +static int mt8365_afe_hw_gain1_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + > + mt8365_afe_enable_main_clk(afe); > + return 0; > +} > + > +static void mt8365_afe_hw_gain1_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + struct mt8365_be_dai_data *be = > + &afe_priv->be_data[dai->id - MT8365_AFE_BACKEND_BASE]; > + > + const unsigned int stream = substream->stream; Why do you const local scalars? What is the point of this variable anyway? > + > + if (be->prepared[stream]) { > + regmap_update_bits(afe->regmap, AFE_GAIN1_CON0, > + AFE_GAIN1_CON0_EN_MASK, 0); > + be->prepared[stream] = false; > + } > + mt8365_afe_disable_main_clk(afe); > +} > + > +static int mt8365_afe_hw_gain1_prepare(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_pcm_runtime * const runtime = substream->runtime; > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + struct mt8365_be_dai_data *be = > + &afe_priv->be_data[dai->id - MT8365_AFE_BACKEND_BASE]; > + > + const unsigned int rate = runtime->rate; > + const unsigned int stream = substream->stream; So this const is everywhere? How does it change it from code safety? No one expects to overwrite such variables anyway, so there is no increase in readability, either. > + int fs; > + unsigned int val1 = 0, val2 = 0; > + > + if (be->prepared[stream]) { > + dev_info(afe->dev, "%s prepared already\n", __func__); > + return 0; > + } > + > + fs = mt8365_afe_fs_timing(rate); > + regmap_update_bits(afe->regmap, AFE_GAIN1_CON0, > + AFE_GAIN1_CON0_MODE_MASK, (unsigned int)fs<<4); > + > + regmap_read(afe->regmap, AFE_GAIN1_CON1, &val1); > + regmap_read(afe->regmap, AFE_GAIN1_CUR, &val2); > + if ((val1 & AFE_GAIN1_CON1_MASK) != (val2 & AFE_GAIN1_CUR_MASK)) > + regmap_update_bits(afe->regmap, AFE_GAIN1_CUR, > + AFE_GAIN1_CUR_MASK, val1); > + > + regmap_update_bits(afe->regmap, AFE_GAIN1_CON0, > + AFE_GAIN1_CON0_EN_MASK, 1); > + be->prepared[stream] = true; > + > + return 0; > +} > + > +static const struct snd_pcm_hardware mt8365_hostless_hardware = { > + .info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | > + SNDRV_PCM_INFO_MMAP_VALID), > + .period_bytes_min = 256, > + .period_bytes_max = 4 * 48 * 1024, > + .periods_min = 2, > + .periods_max = 256, > + .buffer_bytes_max = 8 * 48 * 1024, > + .fifo_size = 0, > +}; > + > +/* dai ops */ > +static int mtk_dai_hostless_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai); > + struct snd_pcm_runtime *runtime = substream->runtime; > + int ret; > + > + snd_soc_set_runtime_hwparams(substream, &mt8365_hostless_hardware); > + > + ret = snd_pcm_hw_constraint_integer(runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) > + dev_info(afe->dev, "snd_pcm_hw_constraint_integer failed\n"); > + return ret; > +} > + > +/* FE DAIs */ > +static const struct snd_soc_dai_ops mt8365_afe_fe_dai_ops = { > + .startup = mt8365_afe_fe_startup, > + .shutdown = mt8365_afe_fe_shutdown, > + .hw_params = mt8365_afe_fe_hw_params, > + .hw_free = mt8365_afe_fe_hw_free, > + .prepare = mt8365_afe_fe_prepare, > + .trigger = mt8365_afe_fe_trigger, > +}; > + > +static const struct snd_soc_dai_ops mt8365_dai_hostless_ops = { > + .startup = mtk_dai_hostless_startup, > +}; > + ... > +static int mt8365_afe_cm2_io_input_mux_get(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + ucontrol->value.integer.value[0] = mCM2Input; > + > + return 0; > +} > + > +static int mt8365_afe_cm2_io_input_mux_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) Run checkpatch --strict > +{ > + struct snd_soc_dapm_context *dapm = > + snd_soc_dapm_kcontrol_dapm(kcontrol); > + struct snd_soc_component *comp = snd_soc_dapm_to_component(dapm); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(comp); > + struct mt8365_afe_private *afe_priv = afe->platform_priv; > + int ret; > + > + mCM2Input = ucontrol->value.enumerated.item[0]; > + > + afe_priv->cm2_mux_input = mCM2Input; > + ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol); > + > + return ret; > +} > + ... > + > +static const struct regmap_config mt8365_afe_regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .max_register = MAX_REGISTER, > + .cache_type = REGCACHE_NONE, > +}; > + > +static irqreturn_t mt8365_afe_irq_handler(int irq, void *dev_id) > +{ > + struct mtk_base_afe *afe = dev_id; > + unsigned int reg_value; > + unsigned int mcu_irq_mask; > + int i, ret; > + > + ret = regmap_read(afe->regmap, AFE_IRQ_MCU_STATUS, &reg_value); > + if (ret) { > + dev_err(afe->dev, "%s irq status err\n", __func__); I can imagine happy user seeing flooded dmesg on each IRQ with these messages... > + reg_value = AFE_IRQ_STATUS_BITS; > + goto err_irq; > + } > + > + ret = regmap_read(afe->regmap, AFE_IRQ_MCU_EN, &mcu_irq_mask); > + if (ret) { > + dev_err(afe->dev, "%s irq mcu_en err\n", __func__); > + reg_value = AFE_IRQ_STATUS_BITS; > + goto err_irq; > + } > + > + /* only clr cpu irq */ > + reg_value &= mcu_irq_mask; > + > + for (i = 0; i < MT8365_AFE_MEMIF_NUM; i++) { > + struct mtk_base_afe_memif *memif = &afe->memif[i]; > + struct mtk_base_afe_irq *mcu_irq; > + > + if (memif->irq_usage < 0) > + continue; > + > + mcu_irq = &afe->irqs[memif->irq_usage]; > + > + if (!(reg_value & (1 << mcu_irq->irq_data->irq_clr_shift))) > + continue; > + > + snd_pcm_period_elapsed(memif->substream); > + } > + > +err_irq: > + /* clear irq */ > + regmap_write(afe->regmap, AFE_IRQ_MCU_CLR, > + reg_value & AFE_IRQ_STATUS_BITS); > + > + return IRQ_HANDLED; > +} > + > +static int __maybe_unused mt8365_afe_runtime_suspend(struct device *dev) > +{ > + return 0; > +} > + > +static int mt8365_afe_runtime_resume(struct device *dev) > +{ > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_suspend(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + struct regmap *regmap = afe->regmap; > + int i; > + > + mt8365_afe_enable_main_clk(afe); > + > + if (!afe->reg_back_up) > + afe->reg_back_up = > + devm_kcalloc(dev, afe->reg_back_up_list_num, > + sizeof(unsigned int), GFP_KERNEL); > + > + for (i = 0; i < afe->reg_back_up_list_num; i++) > + regmap_read(regmap, afe->reg_back_up_list[i], > + &afe->reg_back_up[i]); > + > + mt8365_afe_disable_main_clk(afe); > + > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_resume(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + struct regmap *regmap = afe->regmap; > + int i = 0; > + > + if (!afe->reg_back_up) { > + dev_dbg(dev, "%s no reg_backup\n", __func__); > + return 0; > + } > + > + mt8365_afe_enable_main_clk(afe); > + > + for (i = 0; i < afe->reg_back_up_list_num; i++) > + regmap_write(regmap, afe->reg_back_up_list[i], > + afe->reg_back_up[i]); > + > + mt8365_afe_disable_main_clk(afe); > + > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_dev_runtime_suspend(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + > + dev_dbg(afe->dev, "%s suspend %d %d >>\n", __func__, > + pm_runtime_status_suspended(dev), afe->suspended); Drop silly trace-like prints. Tracing is for this. > + > + if (pm_runtime_status_suspended(dev) || afe->suspended) > + return 0; > + > + mt8365_afe_suspend(dev); > + > + afe->suspended = true; > + > + dev_dbg(afe->dev, "%s <<\n", __func__); Drop silly trace-like prints. Tracing is for this. > + > + return 0; > +} > + > +static int __maybe_unused mt8365_afe_dev_runtime_resume(struct device *dev) > +{ > + struct mtk_base_afe *afe = dev_get_drvdata(dev); > + > + dev_dbg(afe->dev, "%s suspend %d %d >>\n", __func__, > + pm_runtime_status_suspended(dev), afe->suspended); Drop silly trace-like prints. Tracing is for this. > + > + if (pm_runtime_status_suspended(dev) || !afe->suspended) > + return 0; > + > + mt8365_afe_resume(dev); > + > + afe->suspended = false; > + > + dev_dbg(afe->dev, "%s <<\n", __func__); Drop silly trace-like prints. Tracing is for this. > + > + return 0; > +} > + > +static int mt8365_afe_init_registers(struct mtk_base_afe *afe) > +{ > + size_t i; > + > + static struct { > + unsigned int reg; > + unsigned int mask; > + unsigned int val; > + } init_regs[] = { > + { AFE_CONN_24BIT, GENMASK(31, 0), GENMASK(31, 0) }, > + { AFE_CONN_24BIT_1, GENMASK(21, 0), GENMASK(21, 0) }, > + }; > + > + mt8365_afe_enable_main_clk(afe); > + > + for (i = 0; i < ARRAY_SIZE(init_regs); i++) > + regmap_update_bits(afe->regmap, init_regs[i].reg, > + init_regs[i].mask, init_regs[i].val); > + > + mt8365_afe_disable_main_clk(afe); > + > + return 0; > +} > + > +static int mt8365_afe_component_probe(struct snd_soc_component *component) > +{ > + return mtk_afe_add_sub_dai_control(component); > +} > + > +static const struct snd_soc_component_driver mt8365_afe_component = { > + .name = AFE_PCM_NAME, > + .probe = mt8365_afe_component_probe, > + .pointer = mtk_afe_pcm_pointer, > + .pcm_construct = mtk_afe_pcm_new, > +}; > + > +static const struct snd_soc_component_driver mt8365_afe_pcm_component = { > + .name = "mt8365-afe-pcm-dai", > +}; > + > +static int mt8365_dai_memif_register(struct mtk_base_afe *afe) > +{ > + struct mtk_base_afe_dai *dai; > + > + dai = devm_kzalloc(afe->dev, sizeof(*dai), GFP_KERNEL); > + if (!dai) > + return -ENOMEM; > + > + list_add(&dai->list, &afe->sub_dais); > + > + dai->dai_drivers = mt8365_memif_dai_driver; > + dai->num_dai_drivers = ARRAY_SIZE(mt8365_memif_dai_driver); > + > + dai->dapm_widgets = mt8365_memif_widgets; > + dai->num_dapm_widgets = ARRAY_SIZE(mt8365_memif_widgets); > + dai->dapm_routes = mt8365_memif_routes; > + dai->num_dapm_routes = ARRAY_SIZE(mt8365_memif_routes); > + return 0; > +} > + > +typedef int (*dai_register_cb)(struct mtk_base_afe *); > +static const dai_register_cb dai_register_cbs[] = { > + mt8365_dai_pcm_register, > + mt8365_dai_i2s_register, > + mt8365_dai_adda_register, > + mt8365_dai_dmic_register, > + mt8365_dai_memif_register, > +}; > + > +static int mt8365_afe_pcm_dev_probe(struct platform_device *pdev) > +{ > + struct mtk_base_afe *afe; > + struct mt8365_afe_private *afe_priv; > + struct device *dev; > + int ret, i, sel_irq; > + unsigned int irq_id; > + struct resource *res; > + > + afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL); > + if (!afe) > + return -ENOMEM; > + platform_set_drvdata(pdev, afe); > + > + afe->platform_priv = devm_kzalloc(&pdev->dev, sizeof(*afe_priv), > + GFP_KERNEL); > + if (!afe->platform_priv) > + return -ENOMEM; > + > + afe_priv = afe->platform_priv; > + afe->dev = &pdev->dev; > + dev = afe->dev; > + > + spin_lock_init(&afe_priv->afe_ctrl_lock); > + mutex_init(&afe_priv->afe_clk_mutex); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + afe->base_addr = devm_ioremap_resource(&pdev->dev, res); Why not using helper combining these two? > + if (IS_ERR(afe->base_addr)) > + return PTR_ERR(afe->base_addr); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (res) { > + afe_priv->afe_sram_vir_addr = > + devm_ioremap_resource(&pdev->dev, res); > + if (!IS_ERR(afe_priv->afe_sram_vir_addr)) { > + afe_priv->afe_sram_phy_addr = res->start; > + afe_priv->afe_sram_size = resource_size(res); > + } > + } > + > + /* initial audio related clock */ > + ret = mt8365_afe_init_audio_clk(afe); > + if (ret) { > + dev_err(afe->dev, "mt8365_afe_init_audio_clk fail\n"); return dev_err_probe() > + return ret; > + } > + > + afe->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "top_audio_sel", > + afe->base_addr, &mt8365_afe_regmap_config); > + if (IS_ERR(afe->regmap)) > + return PTR_ERR(afe->regmap); > + > + /* memif % irq initialize*/ > + afe->memif_size = MT8365_AFE_MEMIF_NUM; > + afe->memif = devm_kcalloc(afe->dev, afe->memif_size, > + sizeof(*afe->memif), GFP_KERNEL); > + if (!afe->memif) > + return -ENOMEM; > + > + afe->irqs_size = MT8365_AFE_IRQ_NUM; > + afe->irqs = devm_kcalloc(afe->dev, afe->irqs_size, > + sizeof(*afe->irqs), GFP_KERNEL); > + if (!afe->irqs) > + return -ENOMEM; > + > + for (i = 0; i < afe->irqs_size; i++) > + afe->irqs[i].irq_data = &irq_data[i]; > + > + irq_id = platform_get_irq(pdev, 0); > + if (!irq_id) { That does not look correct. 0 is valid. Look at help of this function. > + dev_err(afe->dev, "np %s no irq\n", afe->dev->of_node->name); return dev_err_probe() > + return -ENXIO; > + } > + ret = devm_request_irq(afe->dev, irq_id, mt8365_afe_irq_handler, > + 0, "Afe_ISR_Handle", (void *)afe); > + if (ret) { > + dev_err(afe->dev, "could not request_irq\n"); return dev_err_probe() > + return ret; > + } > + > + /* init sub_dais */ > + INIT_LIST_HEAD(&afe->sub_dais); > + > + for (i = 0; i < ARRAY_SIZE(dai_register_cbs); i++) { > + ret = dai_register_cbs[i](afe); > + if (ret) { > + dev_warn(afe->dev, "dai register i %d fail, ret %d\n", > + i, ret); > + return ret; > + } > + } > + > + /* init dai_driver and component_driver */ > + ret = mtk_afe_combine_sub_dai(afe); > + if (ret) { > + dev_warn(afe->dev, "mtk_afe_combine_sub_dai fail, ret %d\n", > + ret); > + return ret; > + } > + > + for (i = 0; i < afe->memif_size; i++) { > + afe->memif[i].data = &memif_data[i]; > + sel_irq = memif_specified_irqs[i]; > + if (sel_irq >= 0) { > + afe->memif[i].irq_usage = sel_irq; > + afe->memif[i].const_irq = 1; > + afe->irqs[sel_irq].irq_occupyed = true; > + } else { > + afe->memif[i].irq_usage = -1; > + } > + } > + > + afe->mtk_afe_hardware = &mt8365_afe_hardware; > + afe->memif_fs = mt8365_memif_fs; > + afe->irq_fs = mt8365_irq_fs; > + > + pm_runtime_enable(&pdev->dev); > + > + pm_runtime_get_sync(&pdev->dev); > + afe->reg_back_up_list = mt8365_afe_backup_list; > + afe->reg_back_up_list_num = ARRAY_SIZE(mt8365_afe_backup_list); > + afe->runtime_resume = mt8365_afe_runtime_resume; > + afe->runtime_suspend = mt8365_afe_runtime_suspend; > + > + /* open afe pdn for dapm read/write audio register */ > + mt8365_afe_enable_top_cg(afe, MT8365_TOP_CG_AFE); > + > + /* Set 26m parent clk */ > + mt8365_afe_set_clk_parent(afe, > + afe_priv->clocks[MT8365_CLK_TOP_AUD_SEL], > + afe_priv->clocks[MT8365_CLK_CLK26M]); > + > + ret = devm_snd_soc_register_component(&pdev->dev, > + &mt8365_afe_component, NULL, 0); > + if (ret) { > + dev_warn(dev, "err_platform\n"); > + return ret; > + } > + > + ret = devm_snd_soc_register_component(&pdev->dev, > + &mt8365_afe_pcm_component, > + afe->dai_drivers, > + afe->num_dai_drivers); > + if (ret) { > + dev_warn(dev, "err_dai_component\n"); > + return ret; > + } > + > + mt8365_afe_init_registers(afe); > + > + dev_info(&pdev->dev, "MT8365 AFE driver initialized.\n"); Drop simple probe success messages everywhere. They are not needed and they don't bring useful information. Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH 10/18] ASoc: mediatek: mt8365: Add a specific soundcard for EVK

by Krzysztof Kozlowski

On 26/02/2024 15:01, amergnat(a)baylibre.com wrote: > From: Nicolas Belin <nbelin(a)baylibre.com> > > Add a specific soundcard for mt8365-evk. It supports audio jack > in/out, dmics, the amic and lineout. > > Signed-off-by: Nicolas Belin <nbelin(a)baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> ... > +static int mt8365_mt6357_dev_probe(struct platform_device *pdev) > +{ > + struct snd_soc_card *card = &mt8365_mt6357_card; > + struct device *dev = &pdev->dev; > + struct device_node *platform_node; > + struct mt8365_mt6357_priv *priv; > + int i, ret; > + > + card->dev = dev; > + ret = parse_dai_link_info(card); > + if (ret) > + goto err; > + > + platform_node = of_parse_phandle(dev->of_node, "mediatek,platform", 0); > + if (!platform_node) { > + dev_err(dev, "Property 'platform' missing or invalid\n"); > + return -EINVAL; > + } > + > + for (i = 0; i < card->num_links; i++) { > + if (mt8365_mt6357_dais[i].platforms->name) > + continue; > + mt8365_mt6357_dais[i].platforms->of_node = platform_node; > + } > + > + priv = devm_kzalloc(dev, sizeof(struct mt8365_mt6357_priv), > + GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + dev_err(dev, "%s allocate card private data fail %d\n", > + __func__, ret); Don't print anything on memory allocations failures. Run coccinelle/coccicheck to detect such trivial issues. > + return ret; > + } > + > + snd_soc_card_set_drvdata(card, priv); > + > + mt8365_mt6357_gpio_probe(card); > + > + ret = devm_snd_soc_register_card(dev, card); > + if (ret) > + dev_err(dev, "%s snd_soc_register_card fail %d\n", > + __func__, ret); > +err: > + of_node_put(platform_node); > + clean_card_reference(card); > + return ret; > +} > + > +static const struct of_device_id mt8365_mt6357_dt_match[] = { > + { .compatible = "mediatek,mt8365-mt6357", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, mt8365_mt6357_dt_match); > + > +static struct platform_driver mt8365_mt6357_driver = { > + .driver = { > + .name = "mt8365_mt6357", > + .of_match_table = mt8365_mt6357_dt_match, > + .pm = &snd_soc_pm_ops, > + }, > + .probe = mt8365_mt6357_dev_probe, > +}; > + > +module_platform_driver(mt8365_mt6357_driver); > + > +/* Module information */ > +MODULE_DESCRIPTION("MT8365 EVK SoC machine driver"); > +MODULE_AUTHOR("Nicolas Belin <nbelin(a)baylibre.com>"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("mt8365 mt6357 soc card"); This does not look like correct alias. Drop. > Best regards, Krzysztof

1 year, 9 months

1
0
0 0

Re: [PATCH] net: ethernet: ti: am65-cpsw: Add minimal XDP support

by Andrew Lunn

> +static struct sk_buff *am65_cpsw_alloc_skb(struct net_device *ndev, unsigned int len) > +{ > + struct page *page; > + struct sk_buff *skb; > + > + page = dev_alloc_pages(0); You are likely to get better performance if you use the page_pool. When FEC added XDP support, the first set of changes was to make use of page_pool. That improved the drivers performance. Then XDP was added on top. Maybe you can follow that pattern. Andrew

1 year, 9 months

1
0
0 0

Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec

by Mark Brown

On Mon, Feb 26, 2024 at 03:01:50PM +0100, amergnat(a)baylibre.com wrote: > index 000000000000..13e95c227114 > --- /dev/null > +++ b/sound/soc/codecs/mt6357.c > @@ -0,0 +1,1805 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MT6357 ALSA SoC audio codec driver > + * Please use a C++ comment for the whole comment to make it clearer that this is intentional. > +static void set_playback_gpio(struct mt6357_priv *priv, bool enable) > +{ > + if (enable) { > + /* set gpio mosi mode */ > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL); > + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET, GPIO8_MODE_SET_AUD_CLK_MOSI | > + GPIO9_MODE_SET_AUD_DAT_MOSI0 | > + GPIO10_MODE_SET_AUD_DAT_MOSI1 | > + GPIO11_MODE_SET_AUD_SYNC_MOSI); This would be a lot more legible if you worked out the values to set and then had a single set of writes, currently the indentation makes it very hard to read. Similarly for other similar functions. > +static int mt6357_put_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(component); > + struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; > + unsigned int reg; > + int ret; > + > + ret = snd_soc_put_volsw(kcontrol, ucontrol); > + if (ret < 0) > + return ret; > + > + switch (mc->reg) { > + case MT6357_ZCD_CON2: > + regmap_read(priv->regmap, MT6357_ZCD_CON2, &reg); > + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = > + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; > + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = > + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; > + break; It would probably be less code and would definitely be clearer and simpler to just read the values when we need them rather than constatly keeping a cache separate to the register cache. > + /* ul channel swap */ > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), On/off controls should end in Switch. > +static const char * const hslo_mux_map[] = { > + "Open", "DACR", "Playback", "Test mode" > +}; > + > +static int hslo_mux_map_value[] = { > + 0x0, 0x1, 0x2, 0x3, > +}; Why not just use a normal mux here, there's no missing values or reordering? Similarly for other muxes. > +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + unsigned int val; > + > + pr_debug("%s() reg = 0x%x", __func__, reg); > + regmap_read(priv->regmap, reg, &val); > + return val; > +} Remove these, there are vastly more logging facilities as standard in the regmap core. > +/* Reg bit defines */ > +/* MT6357_GPIO_DIR0 */ > +#define GPIO8_DIR_MASK BIT(8) > +#define GPIO8_DIR_INPUT 0 Please namespace your defines, these look very generic.

1 year, 9 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig