Re: [PATCH v4 07/15] dmaengine: ti: k3 PSI-L remote endpoint configuration

[Grygorii Strashko]

On 05/11/2019 12:27, Peter Ujfalusi wrote:
> On 05/11/2019 12.00, Grygorii Strashko wrote:
> > Hi Peter,
> >
> > On 01/11/2019 10:41, Peter Ujfalusi wrote:
> > > In K3 architecture the DMA operates within threads. One end of the thread
> > > is UDMAP, the other is on the peripheral side.
> > >
> > > The UDMAP channel configuration depends on the needs of the remote
> > > endpoint and it can be differ from peripheral to peripheral.
> > >
> > > This patch adds database for am654 and j721e and small API to fetch the
> > > PSI-L endpoint configuration from the database which should only used by
> > > the DMA driver(s).
> > >
> > > Another API is added for native peripherals to give possibility to
> > > pass new
> > > configuration for the threads they are using, which is needed to be
> > > able to
> > > handle changes caused by different firmware loaded for the peripheral for
> > > example.
> >
> > I have no objection to this approach, but ...
> >
> > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
> > > ---
> > >  Â drivers/dma/ti/KconfigÂÂÂÂÂÂÂÂ |ÂÂ 3 +
> > >  Â drivers/dma/ti/MakefileÂÂÂÂÂÂÂ |ÂÂ 1 +
> > >  Â drivers/dma/ti/k3-psil-am654.c | 172 ++++++++++++++++++++++++++
> > >  Â drivers/dma/ti/k3-psil-j721e.c | 219 +++++++++++++++++++++++++++++++++
> > >   drivers/dma/ti/k3-psil-priv.h | 39 ++++++
> > >  Â drivers/dma/ti/k3-psil.cÂÂÂÂÂÂ |Â 97 +++++++++++++++
> > >  Â include/linux/dma/k3-psil.hÂÂÂ |Â 47 +++++++
> > >  Â 7 files changed, 578 insertions(+)
> > >  Â create mode 100644 drivers/dma/ti/k3-psil-am654.c
> > >  Â create mode 100644 drivers/dma/ti/k3-psil-j721e.c
> > >  Â create mode 100644 drivers/dma/ti/k3-psil-priv.h
> > >  Â create mode 100644 drivers/dma/ti/k3-psil.c
> > >  Â create mode 100644 include/linux/dma/k3-psil.h
> >
> > [...]
> >
> > > diff --git a/include/linux/dma/k3-psil.h b/include/linux/dma/k3-psil.h
> > > new file mode 100644
> > > index 000000000000..16e9c8c6f839
> > > --- /dev/null
> > > +++ b/include/linux/dma/k3-psil.h
> > > @@ -0,0 +1,47 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + *Â Copyright (C) 2019 Texas Instruments Incorporated -
> > > http://www.ti.com
> > > + */
> > > +
> > > +#ifndef K3_PSIL_H_
> > > +#define K3_PSIL_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +#define K3_PSIL_DST_THREAD_ID_OFFSET 0x8000
> > > +
> > > +struct device;
> > > +
> > > +/* Channel Throughput Levels */
> > > +enum udma_tp_level {
> > > +ÂÂÂ UDMA_TP_NORMAL = 0,
> > > +ÂÂÂ UDMA_TP_HIGH = 1,
> > > +ÂÂÂ UDMA_TP_ULTRAHIGH = 2,
> > > +ÂÂÂ UDMA_TP_LAST,
> > > +};
> > > +
> > > +enum psil_endpoint_type {
> > > +ÂÂÂ PSIL_EP_NATIVE = 0,
> > > +ÂÂÂ PSIL_EP_PDMA_XY,
> > > +ÂÂÂ PSIL_EP_PDMA_MCAN,
> > > +ÂÂÂ PSIL_EP_PDMA_AASRC,
> > > +};
> > > +
> > > +struct psil_endpoint_config {
> > > +ÂÂÂ enum psil_endpoint_type ep_type;
> > > +
> > > +ÂÂÂ unsigned pkt_mode:1;
> > > +ÂÂÂ unsigned notdpkt:1;
> > > +ÂÂÂ unsigned needs_epib:1;
> > > +ÂÂÂ u32 psd_size;
> > > +ÂÂÂ enum udma_tp_level channel_tpl;
> > > +
> > > +ÂÂÂ /* PDMA properties, valid for PSIL_EP_PDMA_* */
> > > +ÂÂÂ unsigned pdma_acc32:1;
> > > +ÂÂÂ unsigned pdma_burst:1;
> > > +};
> > > +
> > > +int psil_set_new_ep_config(struct device *dev, const char *name,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct psil_endpoint_config *ep_config);
> > > +
> > > +#endif /* K3_PSIL_H_ */
> >
> > I see no user now of this public interface, so I think it better to drop
> > it until
> > there will be real user of it.
>
> The same argument is valid for the glue layer ;)
>
> This is only going to be used by native PSI-L devices and the
> psil_endpoint_config is going to be extended to facilitate their needs
> to give information to the DMA driver on how to set things up.
>
> I would rather avoid churn later on than adding the support from the start.
>
> The point is that the PSI-L endpoint configuration is part of the PSI-L
> peripheral and based on factors these configurations might differ from
> the default one. For example if we want to merge the two physical rx
> channel for sa2ul (so they use the same rflow) or other things we (I)
> can not foresee yet.
> Or if different firmware is loaded for them and it affects their PSI-L
> configuration.

Ok. It's up to you.

otherwise:
Reviewed-by: Grygorii Strashko <grygorii.strashko@xxxxxx>

--
Best regards,
grygorii