On 07/08/2013 02:16 PM, Sergei Shtylyov wrote:
>> not using dmaengine and the network driver (cpsw) which is also using
>> cppi 3.1 is having its own implementation of the cppi-dma part. So I
>> think dma enggine implementation is a must here.
>
> Not at all. I'm not familiar with CPSW but DaVinci EMAC uses CPPI 3.0
> too but it's completely regisyter incompatible with USB. CPPI 3.0
> doesn't seem to be universal DMA engine, hence drivers/dma/ driver
> doesn't seem feasible.
So you suggest to avoid drivers/dma and create drivers/usb
/musb/cpp41-dma.c instead?
cpsw is using davinci_cpdma.c and you say it is completely different
from what musb is using? I just browsed over the spec it looked very
familiar.
>> I will shorten them for the range conflict. usbss is only used for
>> interrupt mask on/off. If there are different, a different compatible
>> string will carry the difference.
>
> You don't quite understand. CPPI 4.1 specification as such (which I
> guess you haven't seen) doesn't include any interrupt registers.
Yes but you do have them somewhere. So all its needs is just the SoC
type which brings the register specification.
>> I think I will also add the usb
>> string to it since a possible network engine will look different in
>> terms of the queue used (which I plan to move away from DT).
>
> There are out of tree platform which uses CPPI 4.1 not only for USB
> but e.g. for Ethernet.
So what? The binding will be different, you get a different register
for interrupt. I'm still not buying that part where you want skip the
dmaengine framework and introduce custom callbacks for this kind of
things.
>>> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
>>>> new file mode 100644
>>>> index 0000000..80dcb56
>>>> --- /dev/null
>>>> +++ b/drivers/dma/cppi41.c
>>>> @@ -0,0 +1,777 @@
> [...]
>
>>>> +struct cppi41_dd {
>>>> + struct dma_device ddev;
>>>> +
>>>> + void *qmgr_scratch;
>>>> + dma_addr_t scratch_phys;
>>>> +
>>>> + struct cppi41_desc *cd;
>>>> + dma_addr_t descs_phys;
>>>> + struct cppi41_channel *chan_busy[ALLOC_DECS_NUM];
>>>> +
>>>> + void __iomem *usbss_mem;
>
>>> Shouldn't be here.
>
>>>> + void __iomem *ctrl_mem;
>>>> + void __iomem *sched_mem;
>>>> + void __iomem *qmgr_mem;
>>>> + unsigned int irq;
>
>>> Shouldn't be here either.
>
>> What is wrong with the four mem pointer here?
>
> I meant only IRQ (interrupt registers are not part of CPPI 4.1 spec).
>
>>>> +static void cppi_writel(u32 val, void *__iomem *mem)
>>>> +{
>>>> + writel(val, mem);
>>>> +}
>>>> +
>>>> +static u32 cppi_readl(void *__iomem *mem)
>>>> +{
>>>> + u32 val;
>>>> + val = readl(mem);
>>>> + return val;
>>>> +}
>
>>> I don't see much sense in these functions. Besides, they should
>>> probably be using __raw_{read|write}().
>
>> I had printk() before posting for debugging. Using raw would require an
>> explicit cache flush before triggering the engine, right?
>
> Don't think so -- I wasn't using it.
>
>>>> +static irqreturn_t cppi41_irq(int irq, void *data)
>>>> +{
>>>> + struct cppi41_dd *cdd = data;
>>>> + struct cppi41_channel *c;
>>>> + u32 status;
>>>> + int i;
>>>> +
>>>> + status = cppi_readl(cdd->usbss_mem + USBSS_IRQ_STATUS);
>>>> + if (!(status & USBSS_IRQ_PD_COMP))
>>>> + return IRQ_NONE;
>
>>> No, this can't be here.
>
>> again. Why not?
>
> This register is not part of CPPI 4.1 spec.
>
>>>> +static u32 get_host_pd0(u32 length)
>>>> +{
>>>> + u32 reg;
>>>> +
>>>> + reg = DESC_TYPE_HOST << DESC_TYPE;
>>>> + reg |= length;
>>>> +
>>>> + return reg;
>>>> +}
>>>> +
>>>> +static u32 get_host_pd1(struct cppi41_channel *c)
>>>> +{
>>>> + u32 reg;
>>>> +
>>>> + reg = 0;
>>>> +
>>>> + return reg;
>>>> +}
>>>> +
>>>> +static u32 get_host_pd2(struct cppi41_channel *c)
>>>> +{
>>>> + u32 reg;
>>>> +
>>>> + reg = 5 << 26; /* USB TYPE */
>
>>> The driver shouldn't be tied to USB at all.
>
>> For now it is USB only. Once we git different types we will have
>> different compatible strings for that. But that shift could by hidden
>> behind a define so to comment could vanish as well.
>
> I repeat: CPPI 4.1 is universal DMA engine. It's a pity that it's
> implemented as a part of USB peripheral on all in-tree platforms but
> it's implemented as an autonomous module on out-of-tree platforms.
It can be still extended to use normal/generic memcpy() operation if
this is supported somewhere. That one here tight to USB and can't do
anything else. I do not start to include a bunch of function pointer if
there is no need it for it. I didn't do anything that it made
impossible to do so, right?
>>>> +static int cppi41_dma_probe(struct platform_device *pdev)
>>>> +{
> [...]
>>>> + irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>>>> + if (!irq)
>>>> + goto err_irq;
>>>> +
>>>> + cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem +
>>>> USBSS_IRQ_ENABLER);
>
>>> Shouldn't be here.
>
>>> [...]
>>>> +static const struct of_device_id cppi41_dma_ids[] = {
>>>> + { .compatible = "ti,am3359-cppi41", },
>
>>> CPPI 4.1 driver should be generic, not SoC specific.
>
>> So you want another driver around it to handle the SoC specific stuff?
>
> This can be handled as part of MUSB DMA driver in our case.
The glue layer you say. Okay. This does not look that bad. I will
try to push it there. But then I need to pass pointers somehow between
cppi41 which is behind dma-engine and this glue layer.
>> As I said earlier, I have only TX completed so for RX channels there is
>> nothing to do. If you want this to be removed wait for the next version
>> which has RX also implemented :)
>
> I don't see why this *if* (which couldn't happen) is in current
> version exactly.
Because RX path will be added. It won't stay like that for ever.
>> Sebastian
>
> PS: Are you aware that TI has written CPPI 4.2 DMA engine driver in
> their Arago project?
>
> http://linux.davincidsp.com/pipermail/davinci-linux-open-source/2013-February/026345.html
No I wasn't. They could bring their code upstream…
> WBR, Sergei
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
