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

[Grygorii Strashko <grygorii.strashko@ti.com>]

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@ti.com>
>>> ---
>>>    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@ti.com>

-- 
Best regards,
grygorii