Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Linus Walleij]

2011/5/25 Mika Westerberg <mika.westerberg@iki.fi>:

> I re-checked the specs and indeed, as discussed previously, it is clear that
> Cirrus (or ARM) copied the interface from other. Even the wording in the
> document is identical in many places. However, in ep93xx User's Guide, there
> is no mention about SSP being AMBA PrimeCell peripheral. For UARTs it is
> mentioned... go figure.
>
> There are differences also, some of the bits in control registers are in
> different places. Interrupt registers are completely different and there is no
> DMA control register at all. The same things we discussed before.

But it really does not matter. The amba-pl022.c driver has all the same
deviations for things called "PL023" that are not pure PL022 either.
I just made the driver a bit more flexible, supporting both.

If the people doing drivers/usb/musb/* had done the same thing we
would have had what, 5 drivers for similar hardware?

> Back then, it wasn't known how the M2M DMA interface (the one used
> by SSP and IDE) was going to be implemented.

But I knew, having just written the needed extensions to dmaengine.
And I said so. But I realize I'm maybe not considered an authoritative
source on the subject, and I'm biased.

> IMHO it is better to have ep93xx implementation on its own driver since it is
> not a real AMBA PL022 peripheral but some weird hack made by Cirrus (although
> my opinions are bit biased).

I disagree because it's IMO at first sight no more deviant than our "PL023"
variants that still use the same pl022 driver.

Is there some inexepensive hardware with EP93xx I can get to fix this
myself?

Yours,
Linus Walleij

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Mika Westerberg]

On Wed, May 25, 2011 at 10:47:14PM +0200, Linus Walleij wrote:
> 
> > IMHO it is better to have ep93xx implementation on its own driver since it is
> > not a real AMBA PL022 peripheral but some weird hack made by Cirrus (although
> > my opinions are bit biased).
> 
> I disagree because it's IMO at first sight no more deviant than our "PL023"
> variants that still use the same pl022 driver.

Ok.

Do you think that we can go forward with this patch series if I drop the last
patch (spi/ep93xx: add DMA support)? This way we will have dmaengine
implementation in place, whatever SPI driver we choose to use with ep93xx.

We can then try to use amba-pl022 driver with ep93xx and see if it works or
not, and how much work is needed. I would like some comments from Hartley and
Ryan also.

> Is there some inexepensive hardware with EP93xx I can get to fix this
> myself?

Check for example Technologic Systems (embeddedarm.com), I have their TS-7260
board which has ep9302.

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Ryan Mallon]

On 26/05/11 16:33, Mika Westerberg wrote:
> On Wed, May 25, 2011 at 10:47:14PM +0200, Linus Walleij wrote:
>>
>>> IMHO it is better to have ep93xx implementation on its own driver since it is
>>> not a real AMBA PL022 peripheral but some weird hack made by Cirrus (although
>>> my opinions are bit biased).
>>
>> I disagree because it's IMO at first sight no more deviant than our "PL023"
>> variants that still use the same pl022 driver.
> 
> Ok.
> 
> Do you think that we can go forward with this patch series if I drop the last
> patch (spi/ep93xx: add DMA support)? This way we will have dmaengine
> implementation in place, whatever SPI driver we choose to use with ep93xx.
> 
> We can then try to use amba-pl022 driver with ep93xx and see if it works or
> not, and how much work is needed. I would like some comments from Hartley and
> Ryan also.

I think if we can modify the amba-pl022 driver, without it turning into
a mess, then we should do that. The ep93xx doesn't really see much
attention, so sharing the code base wherever possible should hopefully
get better review and testing.

The rest of the patch series is fine by me. You can have my

Acked-by: Ryan Mallon <rmallon@gmail.com>

for the ep93xx core and old dma implementation removal parts.

~Ryan

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Linus Walleij]

On Thu, May 26, 2011 at 6:33 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:

> Do you think that we can go forward with this patch series if I drop the last
> patch (spi/ep93xx: add DMA support)? This way we will have dmaengine
> implementation in place, whatever SPI driver we choose to use with ep93xx.

Yes and you don't have to drop any DMA support for the existing driver
for me alone, now that it's in the tree it should be maintained. I only
wanted to raise the issue of code duplication, not block any patches.

> We can then try to use amba-pl022 driver with ep93xx and see if it works or
> not, and how much work is needed. I would like some comments from Hartley and
> Ryan also.

Sure.

Thanks,
Linus Walleij

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Mika Westerberg]

On Thu, May 26, 2011 at 09:15:25AM +0200, Linus Walleij wrote:
> On Thu, May 26, 2011 at 6:33 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
> 
> > Do you think that we can go forward with this patch series if I drop the last
> > patch (spi/ep93xx: add DMA support)? This way we will have dmaengine
> > implementation in place, whatever SPI driver we choose to use with ep93xx.
> 
> Yes and you don't have to drop any DMA support for the existing driver
> for me alone, now that it's in the tree it should be maintained. I only
> wanted to raise the issue of code duplication, not block any patches.

Ok, so I'll keep the last patch for now.

BTW: if we get amba-pl022 driver to work with ep93xx, do you have any ideas
how to proceed from there? I mean, should we convert existing users to use
that driver instead and finally remove the ep93xx_spi driver?

RE: [PATCH 1/5] dmaengine: add ep93xx DMA support

[H Hartley Sweeten]

On Thursday, May 26, 2011 11:38 AM, Mika Westerberg wrote:
> On Thu, May 26, 2011 at 09:15:25AM +0200, Linus Walleij wrote:
>> On Thu, May 26, 2011 at 6:33 AM, Mika Westerberg <mika.westerberg@iki.fi> wrote:
>> 
>>> Do you think that we can go forward with this patch series if I drop the last
>>> patch (spi/ep93xx: add DMA support)? This way we will have dmaengine
>>> implementation in place, whatever SPI driver we choose to use with ep93xx.
>> 
>> Yes and you don't have to drop any DMA support for the existing driver
>> for me alone, now that it's in the tree it should be maintained. I only
>> wanted to raise the issue of code duplication, not block any patches.
>
> Ok, so I'll keep the last patch for now.
>
> BTW: if we get amba-pl022 driver to work with ep93xx, do you have any ideas
> how to proceed from there? I mean, should we convert existing users to use
> that driver instead and finally remove the ep93xx_spi driver?

I think the biggest hurdle with using the amba-pl022 driver is that the ep93xx
spi peripheral does not have the registers with the pid and cid id's.  Without
those I don't think the amba bus driver will be able to match the device to
the driver.

Regards,
Hartley

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Russell King - ARM Linux]

On Thu, May 26, 2011 at 01:48:12PM -0500, H Hartley Sweeten wrote:
> I think the biggest hurdle with using the amba-pl022 driver is that the ep93xx
> spi peripheral does not have the registers with the pid and cid id's.  Without
> those I don't think the amba bus driver will be able to match the device to
> the driver.

I've just committed Linus' patches to allow a provided ID to override
what the bus code finds.

Re: [PATCH 1/5] dmaengine: add ep93xx DMA support

[Linus Walleij]

2011/5/26 H Hartley Sweeten <hartleys@visionengravers.com>:

> I think the biggest hurdle with using the amba-pl022 driver is that the ep93xx
> spi peripheral does not have the registers with the pid and cid id's.  Without
> those I don't think the amba bus driver will be able to match the device to
> the driver.

This is not a problem. We have this code to dynamically create and
register a device with a specific periphid on ux500. It worked even
before my recent patches to amba's bus.c if you have only
zeroes in these registers:

struct amba_device *
dbx500_add_amba_device(const char *name, resource_size_t base,
                       int irq, void *pdata, unsigned int periphid)
{
        struct amba_device *dev;
        int ret;

        dev = kzalloc(sizeof *dev, GFP_KERNEL);
        if (!dev)
                return ERR_PTR(-ENOMEM);

        dev->dev.init_name = name;

        dev->res.start = base;
        dev->res.end = base + SZ_4K - 1;
        dev->res.flags = IORESOURCE_MEM;

        dev->dma_mask = DMA_BIT_MASK(32);
        dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);

        dev->irq[0] = irq;
        dev->irq[1] = NO_IRQ;

        dev->periphid = periphid;

        dev->dev.platform_data = pdata;

        ret = amba_device_register(dev, &iomem_resource);
        if (ret) {
                kfree(dev);
                return ERR_PTR(ret);
        }

        return dev;
}

I'd recommend coming up with some clever numbering scheme
with invented vendor ID.

Yours,
Linus Walleij