Re: [PATCH 2/4] sh: separate DMA headers, used by the legacy and the dmaengine drivers

[Paul Mundt <lethal@linux-sh.org>]

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.. :-)