linux-kernel - RE: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <B56CDBE15CE27145A4B77D2D24263E8524AC79@039-SN1MPN1-003.039d.mgd.msft.net>
Date:	Fri, 6 Dec 2013 07:12:08 +0000
From:	Jingchang Lu <jingchang.lu@...escale.com>
To:	Vinod Koul <vinod.koul@...el.com>
CC:	Mark Rutland <mark.rutland@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"lars@...afoo.de" <lars@...afoo.de>
Subject: RE: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support



> -----Original Message-----
> From: Vinod Koul [mailto:vinod.koul@...el.com]
> Sent: Thursday, November 28, 2013 2:08 PM
> To: Lu Jingchang-B35083
> Cc: Mark Rutland; devicetree@...r.kernel.org; Wang Huan-B18965; linux-
> kernel@...r.kernel.org; shawn.guo@...aro.org; linux-arm-
> kernel@...ts.infradead.org
> Subject: Re: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support
> 
> On Wed, Nov 27, 2013 at 09:38:02AM +0000, Jingchang Lu wrote:
> > > > > > +* DMAMUX
> > > > > > +Required properties:
> > > > >
> > > > > No compatible?
> > > > >
> > > > > Where are DMAMUX nodes expected to live?
> > > > >
> > > > > How to they relate to the eDMA controller in HW? Are they a
> > > > > subcomponent, or a logically separate unit that happens to be
> > > connected?
> > > > [Lu Jingchang-b35083]
> > > > DMAMUX is a multiplexer between dma controller channels and
> peripheral
> > > deivces,
> > > > each DMAMUX provides 16 independently selectable DMA channel
> routers,
> > > and each
> > > > channel router can be assigned to one of the possible peripheral
> DMA
> > > slots.
> > > > So it's not a standalone device, it's just required by the DMA
> > > controller to
> > > > connect the channels and slaves, So it's got by DMA controller's
> > > "fsl,dma-mux" property.
> > > > Thanks!
> > >
> > > Ok.
> > >
> > > I'm not so sure on the way this is described, from the point of view
> of
> > > the device, its DMA channel is wired to the MUX, not to the DMA
> engine
> > > directly:
> > >
> > >                        +-------+
> > >              /---------|DEVICE0|
> > >              |         +-------+
> > > +-----+   +------+
> > > | DMA |===|DMAMUX|
> > > +-----+   +------+
> > >              |         +-------+
> > >              \---------|DEVICE1|
> > >                        +-------+
> > >
> > > If that's the case, I'd expect the DMAMUX to have a #dma-cells and
> > > describe each device as being wired to the mux, and then the mux as
> > > being wired to the DMA. If the MUXes are sub-blocks of the DMA, then
> I'm
> > > not sure why they need to be described at all.
> > >
> > > Currently, the DMA code is handling information that's specific to
> the
> > > MUX (i.e. the channel ID that's specific to the MUX), and that feels
> odd
> > > unless the MUX is a component of the DMA (which if true I'd expect it
> to
> > > be described differently).
> > >
> > Yes, the connection is as your imagination, except for each DMA has two
> MUX.
> > The DMA helper looks the registered DMA engineer for DMA channel
> binding,
> > and the registered DMA engineer is the eDMA node, if binding to DMAMUX,
> > the helper will not find out the dma engineer.
> > The only DMAMUX configuration is programming the slave id into its
> > corresponding register, so its code is handled by the eDMA driver,
> > the DMAMUX is not optional.
> This is fairly common representation of how DMA engines interact with
> slave
> clients via a programmable mux. Now the slave id value in this case
> (which we
> program to mux) needs to be for the client and _not_ for the dmanegine.
> DMA
> engine would have a register to configure the mux value required for the
> transaction.
> 
> The dma engine API provides a way to program the slave id (ie mux value)
> and IMO
> this should be a property of slave device (perhaps part of its dma
> resource) and
> used while programming channel. Making it dma driver property makes no
> sense
> here
Since the DMAMUX node in DTS is not a standalone device but tightly-coupled to the eDMA controller,
Would it be better integrated the DMAMUX info into the eDMA node? Just like the GIC has an dist and
an cpu interface.

edma0: dma-controller@...18000 {
	#dma-cells = <2>;
	compatible = "fsl,vf610-edma";
	reg = <0x40018000 0x2000>,
		<0x40024000 0x1000>,
		<0x40025000 0x1000>;
	interrupts = <0 8 0x04>, <0 9 0x04>;
	interrupt-names = "edma-txrx", "edma-err";
	dma-channels = <32>; 
	clocks = <&clks VF610_CLK_DMAMUX0>,
		<&clks VF610_CLK_DMAMUX1>;
};

And Vinod, I notice that there has been some discussion on the slave_id handling for the SAI audio driver
, which uses the eDMA doing data transfer. Since the driver core infrastructure would automatically call
the DMA engine help function requesting the channel which be done by the driver itself, reducing the work
of individual device driver. So for DMA controller like our eDMA which has slave_id parameter, how about
doing the slave_id configuration during the help call, this can also frees the driver configuring it, and
make the driver compatible with those who has no slave_id. 
Thanks!



Best Regards,
Jingchang