* Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers
2010-02-03 15:03 [PATCH 2/4] sh: separate DMA headers, used by the legacy and the Guennadi Liakhovetski
@ 2010-02-03 23:25 ` Paul Mundt
2010-02-05 9:33 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and Guennadi Liakhovetski
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Paul Mundt @ 2010-02-03 23:25 UTC (permalink / raw)
To: linux-sh
On Wed, Feb 03, 2010 at 04:03:50PM +0100, Guennadi Liakhovetski wrote:
> diff --git a/arch/sh/include/asm/dma-common.h b/arch/sh/include/asm/dma-common.h
> new file mode 100644
> index 0000000..a3456f4
> --- /dev/null
> +++ b/arch/sh/include/asm/dma-common.h
[snip]
> +#define CHCR_DE 0x00000001
> +#define CHCR_TE 0x00000002
> +#define CHCR_IE 0x00000004
> +
> +#include <cpu/dma-register.h>
> +
> +#endif
The easier option would be to rename dma-common.h to dma-register.h,
which would bring it in line with what it's actually pulling in from cpu/.
Even then, these definitions are specific to Hitachi/Renesas-style
on-chip DMACs, so they're not really common across the architecture (this
is why we have dma-sh.h instead of stashing everything in dma.h, although
in retrospect I could have chosen a better name).
> diff --git a/arch/sh/include/asm/sh-dmaengine.h b/arch/sh/include/asm/sh-dmaengine.h
> new file mode 100644
> index 0000000..e127ce5
> --- /dev/null
> +++ b/arch/sh/include/asm/sh-dmaengine.h
> @@ -0,0 +1,71 @@
> +/*
> + * Header for the new SH dmaengine driver
> + *
> + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef SH_DMAENGINE_H
> +#define SH_DMAENGINE_H
> +
Lets just call this asm/dmaengine.h for now. If more dmaengine drivers
come along that want to fight over the namespace, we can shuffle things
around as necessary then.
> +#include <asm/dma-common.h>
> +
> +#define SH_DMAC_MAX_CHANNELS 6
> +
> +/* SuperH DMA mode */
> +#define SHDMA_MIX_IRQ (1 << 0)
> +
With the IRQs being passed through platform data, there shouldn't be any
need for this anymore. If the driver wants to use it internally for
differentiating between muxed vs split out IRQs that's fine, but the
platform data shouldn't care.
> diff --git a/arch/sh/include/cpu-sh3/cpu/dma-register.h b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> new file mode 100644
> index 0000000..ed9c469
> --- /dev/null
> +++ b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> @@ -0,0 +1,40 @@
> +/*
> + * SH3 CPU-specific DMA definitions, used by both DMA drivers
> + *
> + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef DMA_REGISTER_H
> +#define DMA_REGISTER_H
> +
> +#define CHCR_TS_LOW_MASK 0x18
> +#define CHCR_TS_LOW_SHIFT 3
> +#define CHCR_TS_HIGH_MASK 0
> +#define CHCR_TS_HIGH_SHIFT 0
> +
> +#define DMAOR_INIT DMAOR_DME
> +
These should all be part of the platform data.
> +/*
> + * The SuperH DMAC supports a number of transmit sizes, we list them here,
> + * with their respective values as they appear in the CHCR registers.
> + */
> +enum {
> + XMIT_SZ_8BIT,
> + XMIT_SZ_16BIT,
> + XMIT_SZ_32BIT,
> + XMIT_SZ_128BIT,
> +};
> +
> +#define TS_SHIFT { \
> + [XMIT_SZ_8BIT] = 0, \
> + [XMIT_SZ_16BIT] = 1, \
> + [XMIT_SZ_32BIT] = 2, \
> + [XMIT_SZ_128BIT] = 4, \
> +}
> +
> +#define TS_INDEX2VAL(i) (((i) & 3) << CHCR_TS_LOW_SHIFT)
> +
If we compare the simple case with ..
> +/* Transmit sizes and respective CHCR register values */
> +enum {
> + XMIT_SZ_8BIT = 0,
> + XMIT_SZ_16BIT = 1,
> + XMIT_SZ_32BIT = 2,
> + XMIT_SZ_64BIT = 7,
> + XMIT_SZ_128BIT = 3,
> + XMIT_SZ_256BIT = 4,
> + XMIT_SZ_128BIT_BLK = 0xb,
> + XMIT_SZ_256BIT_BLK = 0xc,
> +};
> +
> +/* log2(size / 8) - used to calculate number of transfers */
> +#define TS_SHIFT { \
> + [XMIT_SZ_8BIT] = 0, \
> + [XMIT_SZ_16BIT] = 1, \
> + [XMIT_SZ_32BIT] = 2, \
> + [XMIT_SZ_64BIT] = 3, \
> + [XMIT_SZ_128BIT] = 4, \
> + [XMIT_SZ_256BIT] = 5, \
> + [XMIT_SZ_128BIT_BLK] = 4, \
> + [XMIT_SZ_256BIT_BLK] = 5, \
> +}
> +
> +#define TS_INDEX2VAL(i) ((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> + ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> +
We see that the complex case is a superset, suggesting that it's
worthwhile to make this common.
This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
have visibility of it, and at the same time should be converted to shifts
to permit platform data to have an xmit_sz_valid bitmap or something
similar.
Beyond that, if the xmit sizes are ordered logically by size rather than
by CHCR values then TS_SHIFT can go away and the entire thing can be
calculated dynamically, we would just need a helper routine for
converting a size mask to a CHCR one.
After the above items are done, we should be in a position to kill off
all of the CPU DMA headers.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and
2010-02-03 15:03 [PATCH 2/4] sh: separate DMA headers, used by the legacy and the Guennadi Liakhovetski
2010-02-03 23:25 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers Paul Mundt
@ 2010-02-05 9:33 ` Guennadi Liakhovetski
2010-02-08 1:24 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers Paul Mundt
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2010-02-05 9:33 UTC (permalink / raw)
To: linux-sh
Hi Paul
Thanks for the review. A general question first: many of the headers in
this patch are DMAC specific, not just SuperH DMA specific, are there no
other DMA controllers on DH? Can there ever be any other ones? If so,
maybe we have to choose more specific names, than just dma-register.h or
dmaengine.h... However, so far I don't see any other SH DMA controller
implementations in the kernel - apart from device-specific ones (like
video in / out).
On Thu, 4 Feb 2010, Paul Mundt wrote:
> On Wed, Feb 03, 2010 at 04:03:50PM +0100, Guennadi Liakhovetski wrote:
> > diff --git a/arch/sh/include/asm/dma-common.h b/arch/sh/include/asm/dma-common.h
> > new file mode 100644
> > index 0000000..a3456f4
> > --- /dev/null
> > +++ b/arch/sh/include/asm/dma-common.h
> [snip]
>
> > +#define CHCR_DE 0x00000001
> > +#define CHCR_TE 0x00000002
> > +#define CHCR_IE 0x00000004
> > +
> > +#include <cpu/dma-register.h>
> > +
> > +#endif
>
> The easier option would be to rename dma-common.h to dma-register.h,
> which would bring it in line with what it's actually pulling in from cpu/.
ok
> Even then, these definitions are specific to Hitachi/Renesas-style
> on-chip DMACs, so they're not really common across the architecture (this
> is why we have dma-sh.h instead of stashing everything in dma.h, although
> in retrospect I could have chosen a better name).
Sorry, don't understand. How does putting a file under arch/sh in the "sh"
namespace (dma-sh.h) add any information about its scope?
> > diff --git a/arch/sh/include/asm/sh-dmaengine.h b/arch/sh/include/asm/sh-dmaengine.h
> > new file mode 100644
> > index 0000000..e127ce5
> > --- /dev/null
> > +++ b/arch/sh/include/asm/sh-dmaengine.h
> > @@ -0,0 +1,71 @@
> > +/*
> > + * Header for the new SH dmaengine driver
> > + *
> > + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef SH_DMAENGINE_H
> > +#define SH_DMAENGINE_H
> > +
>
> Lets just call this asm/dmaengine.h for now. If more dmaengine drivers
> come along that want to fight over the namespace, we can shuffle things
> around as necessary then.
ok
>
> > +#include <asm/dma-common.h>
> > +
> > +#define SH_DMAC_MAX_CHANNELS 6
> > +
> > +/* SuperH DMA mode */
> > +#define SHDMA_MIX_IRQ (1 << 0)
> > +
> With the IRQs being passed through platform data, there shouldn't be any
> need for this anymore. If the driver wants to use it internally for
> differentiating between muxed vs split out IRQs that's fine, but the
> platform data shouldn't care.
Actually, do you have an idea why some platforms want or have to use this
shared IRQ scheme? I see 7780 and 7785 using it, although they do have an
IRQ assigned to each channel, the only exception seems to be 7780, which
shares 1 DMA Address Error IRQ between both controllers, but this flag
also multiplexes all channel IRQs... How would you prefer to decide, when
to share IRQs without this flag? Check if IRQs of channel 0 and channel 1
are equal? Or just set the IRQF_SHARED flag always?
> > diff --git a/arch/sh/include/cpu-sh3/cpu/dma-register.h b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> > new file mode 100644
> > index 0000000..ed9c469
> > --- /dev/null
> > +++ b/arch/sh/include/cpu-sh3/cpu/dma-register.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * SH3 CPU-specific DMA definitions, used by both DMA drivers
> > + *
> > + * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef DMA_REGISTER_H
> > +#define DMA_REGISTER_H
> > +
> > +#define CHCR_TS_LOW_MASK 0x18
> > +#define CHCR_TS_LOW_SHIFT 3
> > +#define CHCR_TS_HIGH_MASK 0
> > +#define CHCR_TS_HIGH_SHIFT 0
> > +
> > +#define DMAOR_INIT DMAOR_DME
> > +
> These should all be part of the platform data.
Well, I've spent quite a bit of time thinking about it, and even at first
implemented it like that... But then I decided, that this is something,
that the driver itself can know. I decided, that the proper separation
between platform data and platform-specific driver knowledge would be -
everything, that concerns controller's integration into the system, like
address ranges, interrupt numbers, belongs to platform data, but if
internal register layout differs between platforms, I thought, that should
be driver's internal knowledge. But if you disagree I can switch it over,
no problem. Besides, at least so far, this is not per-controller, but
per-platform specifics.
Besides, these macros are also used by the legacy driver, so, we would end
up with a duplication. They are also used to define TS_INDEX2VAL()...
> > +/*
> > + * The SuperH DMAC supports a number of transmit sizes, we list them here,
> > + * with their respective values as they appear in the CHCR registers.
> > + */
> > +enum {
> > + XMIT_SZ_8BIT,
> > + XMIT_SZ_16BIT,
> > + XMIT_SZ_32BIT,
> > + XMIT_SZ_128BIT,
> > +};
> > +
> > +#define TS_SHIFT { \
> > + [XMIT_SZ_8BIT] = 0, \
> > + [XMIT_SZ_16BIT] = 1, \
> > + [XMIT_SZ_32BIT] = 2, \
> > + [XMIT_SZ_128BIT] = 4, \
> > +}
> > +
> > +#define TS_INDEX2VAL(i) (((i) & 3) << CHCR_TS_LOW_SHIFT)
> > +
> If we compare the simple case with ..
>
> > +/* Transmit sizes and respective CHCR register values */
> > +enum {
> > + XMIT_SZ_8BIT = 0,
> > + XMIT_SZ_16BIT = 1,
> > + XMIT_SZ_32BIT = 2,
> > + XMIT_SZ_64BIT = 7,
> > + XMIT_SZ_128BIT = 3,
> > + XMIT_SZ_256BIT = 4,
> > + XMIT_SZ_128BIT_BLK = 0xb,
> > + XMIT_SZ_256BIT_BLK = 0xc,
> > +};
> > +
> > +/* log2(size / 8) - used to calculate number of transfers */
> > +#define TS_SHIFT { \
> > + [XMIT_SZ_8BIT] = 0, \
> > + [XMIT_SZ_16BIT] = 1, \
> > + [XMIT_SZ_32BIT] = 2, \
> > + [XMIT_SZ_64BIT] = 3, \
> > + [XMIT_SZ_128BIT] = 4, \
> > + [XMIT_SZ_256BIT] = 5, \
> > + [XMIT_SZ_128BIT_BLK] = 4, \
> > + [XMIT_SZ_256BIT_BLK] = 5, \
> > +}
> > +
> > +#define TS_INDEX2VAL(i) ((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> > + ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> > +
> We see that the complex case is a superset, suggesting that it's
> worthwhile to make this common.
Not for all, SH4 has it different, there 0 means 64-byte transfers. But
that's for the enum. The array itself is _obviously_ identical - it's just
the bit-shift for each transfer size - just as my own comment is saying;)
So, yes, that one can be made common.
> This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
> have visibility of it, and at the same time should be converted to shifts
> to permit platform data to have an xmit_sz_valid bitmap or something
> similar.
I think, you misunderstood it. The enum lists CHCR TS bitfield values, so,
we cannot arbitrarily define it. As for validity check - this array is
only used at one
> Beyond that, if the xmit sizes are ordered logically by size rather than
> by CHCR values then TS_SHIFT can go away and the entire thing can be
> calculated dynamically, we would just need a helper routine for
> converting a size mask to a CHCR one.
>
> After the above items are done, we should be in a position to kill off
> all of the CPU DMA headers.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers
2010-02-03 15:03 [PATCH 2/4] sh: separate DMA headers, used by the legacy and the Guennadi Liakhovetski
2010-02-03 23:25 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers Paul Mundt
2010-02-05 9:33 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and Guennadi Liakhovetski
@ 2010-02-08 1:24 ` Paul Mundt
2010-02-08 9:05 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and Guennadi Liakhovetski
2010-02-09 2:34 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers Paul Mundt
4 siblings, 0 replies; 6+ messages in thread
From: Paul Mundt @ 2010-02-08 1:24 UTC (permalink / raw)
To: linux-sh
On Fri, Feb 05, 2010 at 10:33:55AM +0100, Guennadi Liakhovetski wrote:
> Thanks for the review. A general question first: many of the headers in
> this patch are DMAC specific, not just SuperH DMA specific, are there no
> other DMA controllers on DH? Can there ever be any other ones? If so,
> maybe we have to choose more specific names, than just dma-register.h or
> dmaengine.h... However, so far I don't see any other SH DMA controller
> implementations in the kernel - apart from device-specific ones (like
> video in / out).
>
In general all of the on-chip DMACs for system DMA are variations on the
ancient Hitachi DMAC. Some contain more complex features than others (ie,
native scatter-gather, chained transfers, etc, etc.) but they're all
effectively rolled from the same block.
The hordes of other DMACs that find their way in to the design end up
being fairly specific, and will never be suitable for a generic
framework. The corner cases are engines that cascade off of fixed on-chip
DMAC channels and behave like conventional ISA DMA, but no one has built
a system like that since the Dreamcast.
> On Thu, 4 Feb 2010, Paul Mundt wrote:
> > Even then, these definitions are specific to Hitachi/Renesas-style
> > on-chip DMACs, so they're not really common across the architecture (this
> > is why we have dma-sh.h instead of stashing everything in dma.h, although
> > in retrospect I could have chosen a better name).
>
> Sorry, don't understand. How does putting a file under arch/sh in the "sh"
> namespace (dma-sh.h) add any information about its scope?
>
It's a bit confusing, but that particular block has always been fairly
standard SH IP. Having said that, it's not implemented by all of the
CPUs, and it's a wholly optional component for the architecture. Others,
like the ST parts, have their own controllers that have little in common
with the generic SH one. All of the Hitachi/Renesas SH parts evolved from
the old Hitachi DMAC.
> > > +#define SHDMA_MIX_IRQ (1 << 0)
> > > +
> > With the IRQs being passed through platform data, there shouldn't be any
> > need for this anymore. If the driver wants to use it internally for
> > differentiating between muxed vs split out IRQs that's fine, but the
> > platform data shouldn't care.
>
> Actually, do you have an idea why some platforms want or have to use this
> shared IRQ scheme? I see 7780 and 7785 using it, although they do have an
> IRQ assigned to each channel, the only exception seems to be 7780, which
> shares 1 DMA Address Error IRQ between both controllers, but this flag
> also multiplexes all channel IRQs... How would you prefer to decide, when
> to share IRQs without this flag? Check if IRQs of channel 0 and channel 1
> are equal? Or just set the IRQF_SHARED flag always?
>
The shared interrupt scheme is to deal with CPUs that have multiple IRQ
vectors backed by a single masking source. Given that there's no ability
to mask one vector without disabling all of them, we only request_irq()
for each unique masking source and make it shared for the cases where
there are multiple vectors sharing it. This will always be CPU-specific,
so you need to check the intc tables for a specific subtype to figure out
what it can and can't support.
Comparing the interrupt numbers is not sufficient, since they'll be
different. The IRQs are all unique, and are not muxed, they just can't be
masked individually. We could pass the IRQ flow information in through
the IRQ resource (ie, IORESOURCE_IRQ | IRQF_SHARED), but more than likely
most CPUs are going to have this issue, so just using IRQF_SHARED as a
default should be fine, too.
> > > +#define CHCR_TS_LOW_MASK 0x18
> > > +#define CHCR_TS_LOW_SHIFT 3
> > > +#define CHCR_TS_HIGH_MASK 0
> > > +#define CHCR_TS_HIGH_SHIFT 0
> > > +
> > > +#define DMAOR_INIT DMAOR_DME
> > > +
> > These should all be part of the platform data.
>
> Well, I've spent quite a bit of time thinking about it, and even at first
> implemented it like that... But then I decided, that this is something,
> that the driver itself can know. I decided, that the proper separation
> between platform data and platform-specific driver knowledge would be -
> everything, that concerns controller's integration into the system, like
> address ranges, interrupt numbers, belongs to platform data, but if
> internal register layout differs between platforms, I thought, that should
> be driver's internal knowledge. But if you disagree I can switch it over,
> no problem. Besides, at least so far, this is not per-controller, but
> per-platform specifics.
>
Internal register layout will never vary between platforms because the
controller itself is wholly a property of the SoC. Different on-chip
DMACs might have different layouts, but again, all of that is known at
the time that the CPU definitions are being written up. Other than things
like pinmux, the platform itself has very little say about what goes on
with the DMACs.
There are corner cases like the Dreamcast that use reserved bits in
certain system registers to get cascade working, but these should be
treated as the abominations that they are rather than penalizing systems
that aren't crap.
The pros of having all of this as platform data far outweigh any cons.
The few systems that have a problem with this quite frankly brought it on
themselves, and is nothing you should be concerning yourself with.
> Besides, these macros are also used by the legacy driver, so, we would end
> up with a duplication. They are also used to define TS_INDEX2VAL()...
>
Duplication is fine. The legacy driver is its own thing entirely, nothing
in the new driver should be pandering to it. The new driver gives us a
chance to dump all of the legacy garbage and do things right, the fringe
systems that are using the legacy bits can live with duplication. They're
also unlikely to ever support the new driver.
> > > +enum {
> > > + XMIT_SZ_8BIT = 0,
> > > + XMIT_SZ_16BIT = 1,
> > > + XMIT_SZ_32BIT = 2,
> > > + XMIT_SZ_64BIT = 7,
> > > + XMIT_SZ_128BIT = 3,
> > > + XMIT_SZ_256BIT = 4,
> > > + XMIT_SZ_128BIT_BLK = 0xb,
> > > + XMIT_SZ_256BIT_BLK = 0xc,
> > > +};
> > > +
> > > +/* log2(size / 8) - used to calculate number of transfers */
> > > +#define TS_SHIFT { \
> > > + [XMIT_SZ_8BIT] = 0, \
> > > + [XMIT_SZ_16BIT] = 1, \
> > > + [XMIT_SZ_32BIT] = 2, \
> > > + [XMIT_SZ_64BIT] = 3, \
> > > + [XMIT_SZ_128BIT] = 4, \
> > > + [XMIT_SZ_256BIT] = 5, \
> > > + [XMIT_SZ_128BIT_BLK] = 4, \
> > > + [XMIT_SZ_256BIT_BLK] = 5, \
> > > +}
> > > +
> > > +#define TS_INDEX2VAL(i) ((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> > > + ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> > > +
> > We see that the complex case is a superset, suggesting that it's
> > worthwhile to make this common.
>
> Not for all, SH4 has it different, there 0 means 64-byte transfers. But
> that's for the enum. The array itself is _obviously_ identical - it's just
> the bit-shift for each transfer size - just as my own comment is saying;)
> So, yes, that one can be made common.
>
That's ok, as long as it's just one or two CPU families that have such
variations we can just ifdef them in place. If it gets out of hand beyond
that, then that too should be shoved in to platform data.
> > This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
> > have visibility of it, and at the same time should be converted to shifts
> > to permit platform data to have an xmit_sz_valid bitmap or something
> > similar.
>
> I think, you misunderstood it. The enum lists CHCR TS bitfield values, so,
> we cannot arbitrarily define it. As for validity check - this array is
> only used at one
>
I think you misunderstood my comment. My point is that since the TS_SHIFT
is effectively common we could just represent that with a bitmap and then
convert the valid sizes to bit positions for indicating which of the
values is valid for that CPU. A simple wrapper around that to map it out
to the proper CHCR layout would then take care of handling CPU-specific
differences dynamically, which in turn would permit us to kill off all of
this crap from the headers.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and
2010-02-03 15:03 [PATCH 2/4] sh: separate DMA headers, used by the legacy and the Guennadi Liakhovetski
` (2 preceding siblings ...)
2010-02-08 1:24 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers Paul Mundt
@ 2010-02-08 9:05 ` Guennadi Liakhovetski
2010-02-09 2:34 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers Paul Mundt
4 siblings, 0 replies; 6+ messages in thread
From: Guennadi Liakhovetski @ 2010-02-08 9:05 UTC (permalink / raw)
To: linux-sh
On Mon, 8 Feb 2010, Paul Mundt wrote:
> On Fri, Feb 05, 2010 at 10:33:55AM +0100, Guennadi Liakhovetski wrote:
...
> > > > +#define SHDMA_MIX_IRQ (1 << 0)
> > > > +
> > > With the IRQs being passed through platform data, there shouldn't be any
> > > need for this anymore. If the driver wants to use it internally for
> > > differentiating between muxed vs split out IRQs that's fine, but the
> > > platform data shouldn't care.
> >
> > Actually, do you have an idea why some platforms want or have to use this
> > shared IRQ scheme? I see 7780 and 7785 using it, although they do have an
> > IRQ assigned to each channel, the only exception seems to be 7780, which
> > shares 1 DMA Address Error IRQ between both controllers, but this flag
> > also multiplexes all channel IRQs... How would you prefer to decide, when
> > to share IRQs without this flag? Check if IRQs of channel 0 and channel 1
> > are equal? Or just set the IRQF_SHARED flag always?
> >
> The shared interrupt scheme is to deal with CPUs that have multiple IRQ
> vectors backed by a single masking source. Given that there's no ability
> to mask one vector without disabling all of them, we only request_irq()
> for each unique masking source and make it shared for the cases where
> there are multiple vectors sharing it. This will always be CPU-specific,
> so you need to check the intc tables for a specific subtype to figure out
> what it can and can't support.
>
> Comparing the interrupt numbers is not sufficient, since they'll be
> different. The IRQs are all unique, and are not muxed, they just can't be
> masked individually. We could pass the IRQ flow information in through
> the IRQ resource (ie, IORESOURCE_IRQ | IRQF_SHARED), but more than likely
> most CPUs are going to have this issue, so just using IRQF_SHARED as a
> default should be fine, too.
The point is, that the original driver checked for the SHDMA_MIX_IRQ flag,
and if it was set, it used only one IRQ number for all channels and the
error interrupt. So, it affects not only whether IRQF_SHARED is used, but
also what IRQs are requested. If we use different IRQ numbers in platform
data, then we need a way to tell the driver, that only the first one has
to be used. Otherwise we can either specify the same IRQ number already in
platform data, or we do request those different IRQs, in which case the
intc driver will know to use the same mask bit, but then we don't need
IRQF_SHARED?
> > > > +#define CHCR_TS_LOW_MASK 0x18
> > > > +#define CHCR_TS_LOW_SHIFT 3
> > > > +#define CHCR_TS_HIGH_MASK 0
> > > > +#define CHCR_TS_HIGH_SHIFT 0
> > > > +
> > > > +#define DMAOR_INIT DMAOR_DME
> > > > +
> > > These should all be part of the platform data.
> >
> > Well, I've spent quite a bit of time thinking about it, and even at first
> > implemented it like that... But then I decided, that this is something,
> > that the driver itself can know. I decided, that the proper separation
> > between platform data and platform-specific driver knowledge would be -
> > everything, that concerns controller's integration into the system, like
> > address ranges, interrupt numbers, belongs to platform data, but if
> > internal register layout differs between platforms, I thought, that should
> > be driver's internal knowledge. But if you disagree I can switch it over,
> > no problem. Besides, at least so far, this is not per-controller, but
> > per-platform specifics.
> >
> Internal register layout will never vary between platforms because the
> controller itself is wholly a property of the SoC. Different on-chip
> DMACs might have different layouts, but again, all of that is known at
> the time that the CPU definitions are being written up.
Yes, sorry, s/per-platform/per-SoC/. And no, I wasn't aware, that
different DMAC instances on one SoC can have different register layouts.
Just like SCIFs then;)
> Other than things
> like pinmux, the platform itself has very little say about what goes on
> with the DMACs.
>
> There are corner cases like the Dreamcast that use reserved bits in
> certain system registers to get cascade working, but these should be
> treated as the abominations that they are rather than penalizing systems
> that aren't crap.
>
> The pros of having all of this as platform data far outweigh any cons.
> The few systems that have a problem with this quite frankly brought it on
> themselves, and is nothing you should be concerning yourself with.
>
> > Besides, these macros are also used by the legacy driver, so, we would end
> > up with a duplication. They are also used to define TS_INDEX2VAL()...
> >
> Duplication is fine. The legacy driver is its own thing entirely, nothing
> in the new driver should be pandering to it. The new driver gives us a
> chance to dump all of the legacy garbage and do things right, the fringe
> systems that are using the legacy bits can live with duplication. They're
> also unlikely to ever support the new driver.
>
> > > > +enum {
> > > > + XMIT_SZ_8BIT = 0,
> > > > + XMIT_SZ_16BIT = 1,
> > > > + XMIT_SZ_32BIT = 2,
> > > > + XMIT_SZ_64BIT = 7,
> > > > + XMIT_SZ_128BIT = 3,
> > > > + XMIT_SZ_256BIT = 4,
> > > > + XMIT_SZ_128BIT_BLK = 0xb,
> > > > + XMIT_SZ_256BIT_BLK = 0xc,
> > > > +};
> > > > +
> > > > +/* log2(size / 8) - used to calculate number of transfers */
> > > > +#define TS_SHIFT { \
> > > > + [XMIT_SZ_8BIT] = 0, \
> > > > + [XMIT_SZ_16BIT] = 1, \
> > > > + [XMIT_SZ_32BIT] = 2, \
> > > > + [XMIT_SZ_64BIT] = 3, \
> > > > + [XMIT_SZ_128BIT] = 4, \
> > > > + [XMIT_SZ_256BIT] = 5, \
> > > > + [XMIT_SZ_128BIT_BLK] = 4, \
> > > > + [XMIT_SZ_256BIT_BLK] = 5, \
> > > > +}
> > > > +
> > > > +#define TS_INDEX2VAL(i) ((((i) & 3) << CHCR_TS_LOW_SHIFT) | \
> > > > + ((((i) >> 2) & 3) << CHCR_TS_HIGH_SHIFT))
> > > > +
> > > We see that the complex case is a superset, suggesting that it's
> > > worthwhile to make this common.
> >
> > Not for all, SH4 has it different, there 0 means 64-byte transfers. But
> > that's for the enum. The array itself is _obviously_ identical - it's just
> > the bit-shift for each transfer size - just as my own comment is saying;)
> > So, yes, that one can be made common.
> >
> That's ok, as long as it's just one or two CPU families that have such
> variations we can just ifdef them in place. If it gets out of hand beyond
> that, then that too should be shoved in to platform data.
>
> > > This XMIT_SZ_xxx enum should be moved in to the upper level so all CPUs
> > > have visibility of it, and at the same time should be converted to shifts
> > > to permit platform data to have an xmit_sz_valid bitmap or something
> > > similar.
> >
> > I think, you misunderstood it. The enum lists CHCR TS bitfield values, so,
> > we cannot arbitrarily define it. As for validity check - this array is
> > only used at one
> >
> I think you misunderstood my comment. My point is that since the TS_SHIFT
> is effectively common we could just represent that with a bitmap and then
> convert the valid sizes to bit positions for indicating which of the
> values is valid for that CPU. A simple wrapper around that to map it out
> to the proper CHCR layout would then take care of handling CPU-specific
> differences dynamically, which in turn would permit us to kill off all of
> this crap from the headers.
Well, let's see: TS_INDEX2VAL() (and indirectly CHCR_TS_*_SHIFT), and
XMIT_SZ_* are used to initialise .chcr field in platform arrays of struct
sh_dmae_slave_config. So, unless we sant to calculate those fields at
run-time, these have to stay macros. I can add a validity bitmask, but
let's otherwise preserve macros, shall we? But if you say register layout
can be different on different controllers, shall we also be prepared for
different CHCR internal structure? Then yes, we have to pass all that info
in platform data, including the ts_shift array. Shall we do that? And keep
all the macros local to platform code.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers
2010-02-03 15:03 [PATCH 2/4] sh: separate DMA headers, used by the legacy and the Guennadi Liakhovetski
` (3 preceding siblings ...)
2010-02-08 9:05 ` [PATCH 2/4] sh: separate DMA headers, used by the legacy and Guennadi Liakhovetski
@ 2010-02-09 2:34 ` Paul Mundt
4 siblings, 0 replies; 6+ messages in thread
From: Paul Mundt @ 2010-02-09 2:34 UTC (permalink / raw)
To: linux-sh
On Mon, Feb 08, 2010 at 10:05:23AM +0100, Guennadi Liakhovetski wrote:
> On Mon, 8 Feb 2010, Paul Mundt wrote:
> > The shared interrupt scheme is to deal with CPUs that have multiple IRQ
> > vectors backed by a single masking source. Given that there's no ability
> > to mask one vector without disabling all of them, we only request_irq()
> > for each unique masking source and make it shared for the cases where
> > there are multiple vectors sharing it. This will always be CPU-specific,
> > so you need to check the intc tables for a specific subtype to figure out
> > what it can and can't support.
> >
> > Comparing the interrupt numbers is not sufficient, since they'll be
> > different. The IRQs are all unique, and are not muxed, they just can't be
> > masked individually. We could pass the IRQ flow information in through
> > the IRQ resource (ie, IORESOURCE_IRQ | IRQF_SHARED), but more than likely
> > most CPUs are going to have this issue, so just using IRQF_SHARED as a
> > default should be fine, too.
>
> The point is, that the original driver checked for the SHDMA_MIX_IRQ flag,
> and if it was set, it used only one IRQ number for all channels and the
> error interrupt. So, it affects not only whether IRQF_SHARED is used, but
> also what IRQs are requested. If we use different IRQ numbers in platform
> data, then we need a way to tell the driver, that only the first one has
> to be used. Otherwise we can either specify the same IRQ number already in
> platform data, or we do request those different IRQs, in which case the
> intc driver will know to use the same mask bit, but then we don't need
> IRQF_SHARED?
>
I thought that's what I said? We only request_irq() once for a unique
masking source, yes. I don't see precisely what is difficult about this,
there are multiple IRQ resources that can be passed in, if we only get a
single one then we can infer that we only request_irq() once, and at the
same time we can derive the flow control information from the resource
flags.
Whether or not IRQF_SHARED needs to be used or not depends on the CPU
configuration. Just because the DMA events happen to share a masking
source doesn't mean that they are the only ones using it. Most of masking
sources relate to a bit range in a masking register somewhere, and
whatever those bit positions relate to is up to the hardware designers.
Many of the DMA IRQs will share a mask range with IRQs that have no
relationship to the DMA engine at all, so we have no choice but to permit
sharing of the mask source in those cases.
Only the CPU can know what sort of configuration it requires, which is
why the CPU setup code is the only place where the IRQ flow control
information should be specified. Infering too much from the driver side
is going to be pretty fragile, since the driver isn't going to have
enough information to really make flow control decisions on its own.
> > Internal register layout will never vary between platforms because the
> > controller itself is wholly a property of the SoC. Different on-chip
> > DMACs might have different layouts, but again, all of that is known at
> > the time that the CPU definitions are being written up.
>
> Yes, sorry, s/per-platform/per-SoC/. And no, I wasn't aware, that
> different DMAC instances on one SoC can have different register layouts.
> Just like SCIFs then;)
>
Fortunately they're a bit more consistent than SCI/SCIF! I don't think
any of the mass produced CPUs have had to suffer through this mixing and
matching thing yet, but it's certainly something we should try and be
prepared for.
> > I think you misunderstood my comment. My point is that since the TS_SHIFT
> > is effectively common we could just represent that with a bitmap and then
> > convert the valid sizes to bit positions for indicating which of the
> > values is valid for that CPU. A simple wrapper around that to map it out
> > to the proper CHCR layout would then take care of handling CPU-specific
> > differences dynamically, which in turn would permit us to kill off all of
> > this crap from the headers.
>
> Well, let's see: TS_INDEX2VAL() (and indirectly CHCR_TS_*_SHIFT), and
> XMIT_SZ_* are used to initialise .chcr field in platform arrays of struct
> sh_dmae_slave_config. So, unless we sant to calculate those fields at
> run-time, these have to stay macros.
Yes, this is why I mentioned several times that these should be
calculated dynamically. ie, runtime. Given that these macros are already
used in runtime calculation, it really doesn't matter if we make the CHCR
calculation a bit heavier, at the end of the day the ability to kill off
all of those macros makes things a lot more flexible and easier to
maintain going forward.
Having written most of those macros initially, I can assure you that they
were never intended to deal with subtype variations. The original DMA
code had precisely 2 variations, the SH-3 DMA controller and the SH-4.
The Dreamcast built on top of that, and then as soon as SH-4A started,
everyone decided to have a field day with the DMA blocks. None of the
original code was ever intended to deal with the sort of subtype
variation that we have to deal with today, and almost all of that code
predates things like the driver model and all of the DMA interfaces
besides the ISA one.
You really would have been better off just rm -fr'ing all of the old code
before starting out with this series, as you seem to have built up some
sort of attachment to legacy crap that wants to be killed off completely.
These macros are not something we want to preserve unless we really need
to. Making things easier on the legacy code and avoiding some duplication
to that extent is not sufficient grounds for keeping the macros alive.
> But if you say register layout can be different on different
> controllers, shall we also be prepared for different CHCR internal
> structure? Then yes, we have to pass all that info in platform data,
> including the ts_shift array. Shall we do that? And keep all the macros
> local to platform code.
>
That's probably going to be the easiest solution, given that even with
CPU families we see variations between subtypes. The case of individual
CPU subtypes mixing and matching different versions of the same block is
a corner case, but if we can dynamically support something like that,
then we're in pretty good shape for any abomination the hardware folks
decide to throw at us.
You should be happy you got stuck with one of the moderately sane CPUs.. :-)
^ permalink raw reply [flat|nested] 6+ messages in thread