RE: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
Jassi Brar wrote:
> Sent: Wednesday, July 27, 2011 4:58 PM
> To: Boojin Kim
> Cc: Kukjin Kim; Vinod Koul; Mark Brown; Grant Likely; linux-samsung-
> s...@vger.kernel.org; Dan Williams; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
>
> On Wed, Jul 27, 2011 at 10:47 AM, Boojin Kim
> wrote:
> > Jassi Brar wrote:
> >> Sent: Wednesday, July 27, 2011 10:34 AM
> >> To: Boojin Kim
> >> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
> >> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
> >> Likely; Mark Brown
> >> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA
> operations
> >>
> >> On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim
>
> >> wrote:
> >> > Jassi Brar Wrote:
> >> >> Sent: Monday, July 25, 2011 8:52 PM
> >> >> To: Boojin Kim
> >> >> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
> >> >> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
> >> >> Likely; Mark Brown
> >> >> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA
> >> operations
> >> >>
> >> >> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
> >>
> >> >> wrote:
> >> >>
> >> >> > +
> >> >> > +static bool pl330_filter(struct dma_chan *chan, void *param)
> >> >> > +{
> >> >> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri
> >> *)chan-
> >> >> >private;
> >> >> > + unsigned dma_ch = (unsigned)param;
> >> >> > +
> >> >> > + if (peri->peri_id != dma_ch)
> >> >> > + return false;
> >> >> > +
> >> >> > + return true;
> >> >> > +}
> >> >> This is what I meant... if we keep chan_id for paltform assigned
> >> IDs,
> >> >> these filter functions could simply become
> >> >>
> >> >> static bool pl330_filter(struct dma_chan *chan, void *param)
> >> >> {
> >> >> return chan->chan_id == param
> >> >> }
> >> >>
> >> >> And ideally in the long run, we could just drop the filter
> callback
> >> >> and add expected channel ID to the request_channel call.
> >> > The chan_id is set by dmaengine. So, We don't use it to hold the
> >> user specific
> >> > Id.
> >> Not for long.
> >> See https://lkml.org/lkml/2011/7/26/245
> >>
> >>
> >> >> > +
> >> >> > +static inline int s3c_dma_trigger(unsigned ch)
> >> >> > +{
> >> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
> >> >> > +}
> >> >> > +
> >> >> > +static inline int s3c_dma_started(unsigned ch)
> >> >> > +{
> >> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
> >> >> > +}
> >> >> > +
> >> >> > +static inline int s3c_dma_flush(unsigned ch)
> >> >> > +{
> >> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
> >> >> > +}
> >> >> > +
> >> >> > +static inline int s3c_dma_stop(unsigned ch)
> >> >> > +{
> >> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
> >> >> > +}
> >> >> > +
> >> >> > +static struct samsung_dma_ops s3c_dma_ops = {
> >> >> > + .request= s3c_dma_request,
> >> >> > + .release= s3c_dma_release,
> >> >> > + .prepare= s3c_dma_prepare,
> >> >> > + .trigger= s3c_dma_trigger,
> >> >> > + .started= s3c_dma_started,
> >> >> > + .flush = s3c_dma_flush,
> >> >> > + .stop = s3c_dma_stop,
> >> >>
> >> >> These last 4 should be gnereallized into one callback with OP
> >> argument.
> >> > I don't have any idea about it. Can you explain it in more detail?
> >>
> >> static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or
> >> similar*/ op)
> >> {
> >> return s3c2410_dma_ctrl(ch, op);
> >> }
> >>
> >> static struct samsung_dma_ops s3c_dma_ops = {
> >> .request= s3c_dma_request,
> >> .release= s3c_dma_release,
> >> .prepare= s3c_dma_prepare,
> >> .control= s3c_dma_control,
> > 'Struct samsung_dma_ops' is used for both 'S3C-DMA-OPS' for 's3c
> dma' and
> > 'DMA-OPS' for 'dmaengine'.
> > If I modify "Struct samsung_dma_ops" as your guide, It gives un-
> expectable
> > effect to DMA-OPS.
> > Actually, We don't want the client with 'DMA-OPS' uses 'enum
> s3c2410_chan_op'
> > operation anymore.
>
> " enum s3c2410_chan_op /* or similar */ " <<<< note 'or similar'
>
> Defining a callback for each value of the argument doesn't make sense.
Yes, The command may not be 'enum s3c2410_chan_op'. But it's very similar with
'enum s3c2410_chan_op'.
So, It brings same result that I sad.
Actually, I quietly agree with your comment. If the 'struct samsung_dma_ops'
is only used for 's3c-dma-ops', I would address your comment. But, 'struct
samsung_dma_ops' is used for all Samsung dma clients. I aim that 'struct
samsung_dma_ops' provide the DMA clients with 'dmaengine' friendly DMA
interface without any other command.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Wed, Jul 27, 2011 at 10:47 AM, Boojin Kim wrote:
> Jassi Brar wrote:
>> Sent: Wednesday, July 27, 2011 10:34 AM
>> To: Boojin Kim
>> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
>> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
>> Likely; Mark Brown
>> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
>>
>> On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim
>> wrote:
>> > Jassi Brar Wrote:
>> >> Sent: Monday, July 25, 2011 8:52 PM
>> >> To: Boojin Kim
>> >> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
>> >> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
>> >> Likely; Mark Brown
>> >> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA
>> operations
>> >>
>> >> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
>>
>> >> wrote:
>> >>
>> >> > +
>> >> > +static bool pl330_filter(struct dma_chan *chan, void *param)
>> >> > +{
>> >> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri
>> *)chan-
>> >> >private;
>> >> > + unsigned dma_ch = (unsigned)param;
>> >> > +
>> >> > + if (peri->peri_id != dma_ch)
>> >> > + return false;
>> >> > +
>> >> > + return true;
>> >> > +}
>> >> This is what I meant... if we keep chan_id for paltform assigned
>> IDs,
>> >> these filter functions could simply become
>> >>
>> >> static bool pl330_filter(struct dma_chan *chan, void *param)
>> >> {
>> >> return chan->chan_id == param
>> >> }
>> >>
>> >> And ideally in the long run, we could just drop the filter callback
>> >> and add expected channel ID to the request_channel call.
>> > The chan_id is set by dmaengine. So, We don't use it to hold the
>> user specific
>> > Id.
>> Not for long.
>> See https://lkml.org/lkml/2011/7/26/245
>>
>>
>> >> > +
>> >> > +static inline int s3c_dma_trigger(unsigned ch)
>> >> > +{
>> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
>> >> > +}
>> >> > +
>> >> > +static inline int s3c_dma_started(unsigned ch)
>> >> > +{
>> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
>> >> > +}
>> >> > +
>> >> > +static inline int s3c_dma_flush(unsigned ch)
>> >> > +{
>> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
>> >> > +}
>> >> > +
>> >> > +static inline int s3c_dma_stop(unsigned ch)
>> >> > +{
>> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
>> >> > +}
>> >> > +
>> >> > +static struct samsung_dma_ops s3c_dma_ops = {
>> >> > + .request = s3c_dma_request,
>> >> > + .release = s3c_dma_release,
>> >> > + .prepare = s3c_dma_prepare,
>> >> > + .trigger = s3c_dma_trigger,
>> >> > + .started = s3c_dma_started,
>> >> > + .flush = s3c_dma_flush,
>> >> > + .stop = s3c_dma_stop,
>> >>
>> >> These last 4 should be gnereallized into one callback with OP
>> argument.
>> > I don't have any idea about it. Can you explain it in more detail?
>>
>> static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or
>> similar*/ op)
>> {
>> return s3c2410_dma_ctrl(ch, op);
>> }
>>
>> static struct samsung_dma_ops s3c_dma_ops = {
>> .request = s3c_dma_request,
>> .release = s3c_dma_release,
>> .prepare = s3c_dma_prepare,
>> .control = s3c_dma_control,
> 'Struct samsung_dma_ops' is used for both 'S3C-DMA-OPS' for 's3c dma' and
> 'DMA-OPS' for 'dmaengine'.
> If I modify "Struct samsung_dma_ops" as your guide, It gives un-expectable
> effect to DMA-OPS.
> Actually, We don't want the client with 'DMA-OPS' uses 'enum s3c2410_chan_op'
> operation anymore.
" enum s3c2410_chan_op /* or similar */ " <<<< note 'or similar'
Defining a callback for each value of the argument doesn't make sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
Jassi Brar wrote:
> Sent: Wednesday, July 27, 2011 10:34 AM
> To: Boojin Kim
> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
> Likely; Mark Brown
> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
>
> On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim
> wrote:
> > Jassi Brar Wrote:
> >> Sent: Monday, July 25, 2011 8:52 PM
> >> To: Boojin Kim
> >> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
> >> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
> >> Likely; Mark Brown
> >> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA
> operations
> >>
> >> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
>
> >> wrote:
> >>
> >> > +
> >> > +static bool pl330_filter(struct dma_chan *chan, void *param)
> >> > +{
> >> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri
> *)chan-
> >> >private;
> >> > + unsigned dma_ch = (unsigned)param;
> >> > +
> >> > + if (peri->peri_id != dma_ch)
> >> > + return false;
> >> > +
> >> > + return true;
> >> > +}
> >> This is what I meant... if we keep chan_id for paltform assigned
> IDs,
> >> these filter functions could simply become
> >>
> >> static bool pl330_filter(struct dma_chan *chan, void *param)
> >> {
> >> return chan->chan_id == param
> >> }
> >>
> >> And ideally in the long run, we could just drop the filter callback
> >> and add expected channel ID to the request_channel call.
> > The chan_id is set by dmaengine. So, We don't use it to hold the
> user specific
> > Id.
> Not for long.
> See https://lkml.org/lkml/2011/7/26/245
>
>
> >> > +
> >> > +static inline int s3c_dma_trigger(unsigned ch)
> >> > +{
> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
> >> > +}
> >> > +
> >> > +static inline int s3c_dma_started(unsigned ch)
> >> > +{
> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
> >> > +}
> >> > +
> >> > +static inline int s3c_dma_flush(unsigned ch)
> >> > +{
> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
> >> > +}
> >> > +
> >> > +static inline int s3c_dma_stop(unsigned ch)
> >> > +{
> >> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
> >> > +}
> >> > +
> >> > +static struct samsung_dma_ops s3c_dma_ops = {
> >> > + .request= s3c_dma_request,
> >> > + .release= s3c_dma_release,
> >> > + .prepare= s3c_dma_prepare,
> >> > + .trigger= s3c_dma_trigger,
> >> > + .started= s3c_dma_started,
> >> > + .flush = s3c_dma_flush,
> >> > + .stop = s3c_dma_stop,
> >>
> >> These last 4 should be gnereallized into one callback with OP
> argument.
> > I don't have any idea about it. Can you explain it in more detail?
>
> static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or
> similar*/ op)
> {
> return s3c2410_dma_ctrl(ch, op);
> }
>
> static struct samsung_dma_ops s3c_dma_ops = {
> .request= s3c_dma_request,
> .release= s3c_dma_release,
> .prepare= s3c_dma_prepare,
> .control= s3c_dma_control,
'Struct samsung_dma_ops' is used for both 'S3C-DMA-OPS' for 's3c dma' and
'DMA-OPS' for 'dmaengine'.
If I modify "Struct samsung_dma_ops" as your guide, It gives un-expectable
effect to DMA-OPS.
Actually, We don't want the client with 'DMA-OPS' uses 'enum s3c2410_chan_op'
operation anymore.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim wrote:
> Jassi Brar Wrote:
>> Sent: Monday, July 25, 2011 8:52 PM
>> To: Boojin Kim
>> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
>> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
>> Likely; Mark Brown
>> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
>>
>> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
>> wrote:
>>
>> > +
>> > +static bool pl330_filter(struct dma_chan *chan, void *param)
>> > +{
>> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan-
>> >private;
>> > + unsigned dma_ch = (unsigned)param;
>> > +
>> > + if (peri->peri_id != dma_ch)
>> > + return false;
>> > +
>> > + return true;
>> > +}
>> This is what I meant... if we keep chan_id for paltform assigned IDs,
>> these filter functions could simply become
>>
>> static bool pl330_filter(struct dma_chan *chan, void *param)
>> {
>> return chan->chan_id == param
>> }
>>
>> And ideally in the long run, we could just drop the filter callback
>> and add expected channel ID to the request_channel call.
> The chan_id is set by dmaengine. So, We don't use it to hold the user specific
> Id.
Not for long.
See https://lkml.org/lkml/2011/7/26/245
>> > +
>> > +static inline int s3c_dma_trigger(unsigned ch)
>> > +{
>> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
>> > +}
>> > +
>> > +static inline int s3c_dma_started(unsigned ch)
>> > +{
>> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
>> > +}
>> > +
>> > +static inline int s3c_dma_flush(unsigned ch)
>> > +{
>> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
>> > +}
>> > +
>> > +static inline int s3c_dma_stop(unsigned ch)
>> > +{
>> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
>> > +}
>> > +
>> > +static struct samsung_dma_ops s3c_dma_ops = {
>> > + .request = s3c_dma_request,
>> > + .release = s3c_dma_release,
>> > + .prepare = s3c_dma_prepare,
>> > + .trigger = s3c_dma_trigger,
>> > + .started = s3c_dma_started,
>> > + .flush = s3c_dma_flush,
>> > + .stop = s3c_dma_stop,
>>
>> These last 4 should be gnereallized into one callback with OP argument.
> I don't have any idea about it. Can you explain it in more detail?
static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or similar*/ op)
{
return s3c2410_dma_ctrl(ch, op);
}
static struct samsung_dma_ops s3c_dma_ops = {
.request= s3c_dma_request,
.release= s3c_dma_release,
.prepare= s3c_dma_prepare,
.control= s3c_dma_control,
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 11:44 PM, Russell King - ARM Linux wrote: > > 1. Arrange for individual DMA engine drivers to provide a filter > function - eg, pl08x_filter_id() for pl08x channels. > > 2. Have the filter function check chan->device->dev->driver == its > own struct driver as the very first thing, and reject on failure. > > 3. Have its own matching algorithm using whatever data is appropriate > for the DMA engine driver. IMHO it should be possible to have client drivers not use platform specific details while choosing a channel. Otherwise, they are not really 'generic' 'platform-independent' client drivers. >> I hope we agree, clients (esp 'generic' ones of the future) >> should not ask for channels from a particular dmac. >> They should simply tell DMA API, what capabilities they expect >> from the allocated channel - of whatever index on whichever dmac. >> And the platform should have specified capabilities of a channel >> while introducing it to the DMA API. Yes, imho, DMA API should >> only work with a pool of channels and not dmacs - it shouldn't >> matter from which dmac a channel comes. > > You're quite clearly confused. "DMA API" has not very much to do with > the subject we're discussing. The "DMA API" is the API for mapping and > unmapping buffers for DMA. It has not much to do with the DMA engine > API. So I'm not going to bother trying to decode what you said above > until you start using the right terminology. I'm a stickler for that, > get used to it. > > No point talking about how round bananas are when you're actually > discussing orange fruit. Sorry, my carelessness. I should have written DMAENGINE API, instead of DMA API. If you want I can repost with s/DMA API/DMAENGINE API/ > >> So when a client asks for a channel, the dmaengine would return >> the first free channel that has just as much the requested capability. > > No. For slave DMA, peripheral drivers normally need a specific > channel on the DMA controller. Of course they do. And It is the responsibility of platform to encode such information as a 'capability' of the channel. Capability to reach a particular peripheral, say. > Peripheral drivers normally do not > know which channel they need, or even which DMA controller they > require. That information is specific to the platform and SoC. For that very reason I proposed, say, if MMC client driver is probed for 2nd instance of MMC controller, it should ask for the following capabilities (MMC << TYPE_SHFT) | (2 << INST_SHFT)//I don't show other fields for simplicity It is the job of _platform_ to set TYPE and INSTANCE fields to 'MMC' and 2 resp _only_ for the channel(s) that can reach its MMC_2 controller. That way, when the client 'generically' asks for DMA_MMC_2, it will be given only one of those channels that can talk to second instance of MMC controller. >> That is, the generic filter function would look equivalent of :- >> >> return is_free(chan) && (cap_requested == cap_of(chan) & cap_requested) >> >> >> Now it all boils down to defining the set of _practically_ possible >> capabilities >> and a method to exchange them between the API, Client and DMAC drivers. > > What are these capabilities? I had mentioned the list below under 'Assuming'. We may add more. > >> The above is the overall approach. >> >> The following is my idea of implementing it as of now, I am open to >> suggestions. >> >> Assuming :- >> *** >> There are no more than 256 types of DMA'able devices >> (MMC, USB, SPI etc) -- [8bits] >> >> A type of device never has more than 16 instances in a platform >> (MMC-0, MMC-1, MMC-2 etc) -- [4bits] >> >> Mem->Mem, Mem->Dev, Dev->Mem, Dev->Dev capability in [4bits] > > No. You're being far too specific to your own DMA controller. Many > DMA controllers (eg, PL08x) have a number of physical channels, all of > which can be programmed for M2M, M2P, P2M and P2P. There is nothing > in the design which restricts what a channel can do. I am not being specific, at least not on this point. Btw, I have worked on pl080 as well and I have tried to be not specific to not only pl330 but also pl080. If you notice I have assigned 4bits to the field. Each of the 4 bits can be set independently. So, a capable pl08x channel can set all 4 bits to indicate that it can do all 4 types - M2M, M2P, P2M and P2P. >> Max_Burst_Len for any channel is less than 64KB, in power of two [4bits] >> Support programmable Max_Burst_Len(tunable in geometric steps of 2) [1bit] >> >> Max_RW_width/fifo_width is less than 128, in power of two -- [3bits] >> Support programmable Max_RW_Width(tunable in geometric steps of 2) [1bit] >> >> Finally, No platform has more than 128 channels with identicial >> capabilities [7bits] > > There is absolutely no way that this will work in reality - are you on > a different planet? As has already been described to you, MMCI can > use one DMA channel, and programs it according to the DMA direction, > and
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 11:13:35PM +0530, Jassi Brar wrote: > Before we start, let me be clear that I don't say it's all possible > today as such. It _is_ possible today. 1. Arrange for individual DMA engine drivers to provide a filter function - eg, pl08x_filter_id() for pl08x channels. 2. Have the filter function check chan->device->dev->driver == its own struct driver as the very first thing, and reject on failure. 3. Have its own matching algorithm using whatever data is appropriate for the DMA engine driver. 4. Platforms (or SoC code) provide the match function plus match data via platform data to each driver. That is essentially (except for (2)) how we deal with differing DMA engine drivers with AMBA peripherals, and it seems to work remarkably well. > I hope we agree, clients (esp 'generic' ones of the future) > should not ask for channels from a particular dmac. > They should simply tell DMA API, what capabilities they expect > from the allocated channel - of whatever index on whichever dmac. > And the platform should have specified capabilities of a channel > while introducing it to the DMA API. Yes, imho, DMA API should > only work with a pool of channels and not dmacs - it shouldn't > matter from which dmac a channel comes. You're quite clearly confused. "DMA API" has not very much to do with the subject we're discussing. The "DMA API" is the API for mapping and unmapping buffers for DMA. It has not much to do with the DMA engine API. So I'm not going to bother trying to decode what you said above until you start using the right terminology. I'm a stickler for that, get used to it. No point talking about how round bananas are when you're actually discussing orange fruit. > So when a client asks for a channel, the dmaengine would return > the first free channel that has just as much the requested capability. No. For slave DMA, peripheral drivers normally need a specific channel on the DMA controller. Peripheral drivers normally do not know which channel they need, or even which DMA controller they require. That information is specific to the platform and SoC. So, returning the "first free channel that matches the capability" is senseless - insane - for slave DMA. > That is, the generic filter function would look equivalent of :- > > return is_free(chan) && (cap_requested == cap_of(chan) & cap_requested) > > > Now it all boils down to defining the set of _practically_ possible > capabilities > and a method to exchange them between the API, Client and DMAC drivers. What are these capabilities? > The above is the overall approach. > > The following is my idea of implementing it as of now, I am open to > suggestions. > > Assuming :- > *** > There are no more than 256 types of DMA'able devices > (MMC, USB, SPI etc) -- [8bits] > > A type of device never has more than 16 instances in a platform > (MMC-0, MMC-1, MMC-2 etc) -- [4bits] > > Mem->Mem, Mem->Dev, Dev->Mem, Dev->Dev capability in [4bits] No. You're being far too specific to your own DMA controller. Many DMA controllers (eg, PL08x) have a number of physical channels, all of which can be programmed for M2M, M2P, P2M and P2P. There is nothing in the design which restricts what a channel can do. So to put that as the basis for a generic match algorithm is absolutely senseless. > Max_Burst_Len for any channel is less than 64KB, in power of two [4bits] > Support programmable Max_Burst_Len(tunable in geometric steps of 2) [1bit] > > Max_RW_width/fifo_width is less than 128, in power of two -- [3bits] > Support programmable Max_RW_Width(tunable in geometric steps of 2) [1bit] > > Finally, No platform has more than 128 channels with identicial > capabilities [7bits] There is absolutely no way that this will work in reality - are you on a different planet? As has already been described to you, MMCI can use one DMA channel, and programs it according to the DMA direction, and has in the past adjusted the burst size according to what's going on with the driver. With your model, it would have to claim N channels from the DMA engine for all possible different configurations. That's utterly unworkable and unrealistic. So no, I don't think your idea of 'capabilities' is any solution. It just creates yet more problems - more than it solves. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 1:32 PM, Russell King - ARM Linux
wrote:
> On Mon, Jul 25, 2011 at 05:21:44PM +0530, Jassi Brar wrote:
>> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim wrote:
>>
>> > +
>> > +static bool pl330_filter(struct dma_chan *chan, void *param)
>> > +{
>> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri
>> > *)chan->private;
>> > + unsigned dma_ch = (unsigned)param;
>> > +
>> > + if (peri->peri_id != dma_ch)
>> > + return false;
>> > +
>> > + return true;
>> > +}
>> This is what I meant... if we keep chan_id for paltform assigned IDs,
>> these filter functions could simply become
>>
>> static bool pl330_filter(struct dma_chan *chan, void *param)
>> {
>> return chan->chan_id == param
>> }
>>
>> And ideally in the long run, we could just drop the filter callback
>> and add expected channel ID to the request_channel call.
>
> So what if you have a PL080 and PL330 drivers registered with the DMA
> engine code, and you need to match a specific PL330 channel?
>
Before we start, let me be clear that I don't say it's all possible
today as such.
We will need changes to the API and modifications to the damc/client
drivers (which are already misusing the API as it turns out).
I hope we agree, clients (esp 'generic' ones of the future)
should not ask for channels from a particular dmac.
They should simply tell DMA API, what capabilities they expect
from the allocated channel - of whatever index on whichever dmac.
And the platform should have specified capabilities of a channel
while introducing it to the DMA API. Yes, imho, DMA API should
only work with a pool of channels and not dmacs - it shouldn't
matter from which dmac a channel comes.
So when a client asks for a channel, the dmaengine would return
the first free channel that has just as much the requested capability.
That is, the generic filter function would look equivalent of :-
return is_free(chan) && (cap_requested == cap_of(chan) & cap_requested)
Now it all boils down to defining the set of _practically_ possible capabilities
and a method to exchange them between the API, Client and DMAC drivers.
The above is the overall approach.
The following is my idea of implementing it as of now, I am open to suggestions.
Assuming :-
***
There are no more than 256 types of DMA'able devices
(MMC, USB, SPI etc) -- [8bits]
A type of device never has more than 16 instances in a platform
(MMC-0, MMC-1, MMC-2 etc) -- [4bits]
Mem->Mem, Mem->Dev, Dev->Mem, Dev->Dev capability in [4bits]
Max_Burst_Len for any channel is less than 64KB, in power of two [4bits]
Support programmable Max_Burst_Len(tunable in geometric steps of 2) [1bit]
Max_RW_width/fifo_width is less than 128, in power of two -- [3bits]
Support programmable Max_RW_Width(tunable in geometric steps of 2) [1bit]
Finally, No platform has more than 128 channels with identicial
capabilities [7bits]
For a channel, when specific values of these fields are appended together,
we get a unqiue 32-bits value called 'chan_id'
Though we can use 64-bits to store the capabilities if we think number
or size of these fields need to be increased.
Thanks,
Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
Jassi Brar Wrote:
> Sent: Monday, July 25, 2011 8:52 PM
> To: Boojin Kim
> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
> s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
> Likely; Mark Brown
> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
>
> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
> wrote:
>
> > +
> > +static bool pl330_filter(struct dma_chan *chan, void *param)
> > +{
> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan-
> >private;
> > + unsigned dma_ch = (unsigned)param;
> > +
> > + if (peri->peri_id != dma_ch)
> > + return false;
> > +
> > + return true;
> > +}
> This is what I meant... if we keep chan_id for paltform assigned IDs,
> these filter functions could simply become
>
> static bool pl330_filter(struct dma_chan *chan, void *param)
> {
> return chan->chan_id == param
> }
>
> And ideally in the long run, we could just drop the filter callback
> and add expected channel ID to the request_channel call.
The chan_id is set by dmaengine. So, We don't use it to hold the user specific
Id.
>
>
>
> > +
> > +static inline int s3c_dma_trigger(unsigned ch)
> > +{
> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
> > +}
> > +
> > +static inline int s3c_dma_started(unsigned ch)
> > +{
> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
> > +}
> > +
> > +static inline int s3c_dma_flush(unsigned ch)
> > +{
> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
> > +}
> > +
> > +static inline int s3c_dma_stop(unsigned ch)
> > +{
> > + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
> > +}
> > +
> > +static struct samsung_dma_ops s3c_dma_ops = {
> > + .request= s3c_dma_request,
> > + .release= s3c_dma_release,
> > + .prepare= s3c_dma_prepare,
> > + .trigger= s3c_dma_trigger,
> > + .started= s3c_dma_started,
> > + .flush = s3c_dma_flush,
> > + .stop = s3c_dma_stop,
>
> These last 4 should be gnereallized into one callback with OP argument.
I don't have any idea about it. Can you explain it in more detail?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Mon, Jul 25, 2011 at 05:21:44PM +0530, Jassi Brar wrote:
> On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim wrote:
>
> > +
> > +static bool pl330_filter(struct dma_chan *chan, void *param)
> > +{
> > + struct dma_pl330_peri *peri = (struct dma_pl330_peri
> > *)chan->private;
> > + unsigned dma_ch = (unsigned)param;
> > +
> > + if (peri->peri_id != dma_ch)
> > + return false;
> > +
> > + return true;
> > +}
> This is what I meant... if we keep chan_id for paltform assigned IDs,
> these filter functions could simply become
>
> static bool pl330_filter(struct dma_chan *chan, void *param)
> {
> return chan->chan_id == param
> }
>
> And ideally in the long run, we could just drop the filter callback
> and add expected channel ID to the request_channel call.
So what if you have a PL080 and PL330 drivers registered with the DMA
engine code, and you need to match a specific PL330 channel?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim wrote:
> +
> +static bool pl330_filter(struct dma_chan *chan, void *param)
> +{
> + struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan->private;
> + unsigned dma_ch = (unsigned)param;
> +
> + if (peri->peri_id != dma_ch)
> + return false;
> +
> + return true;
> +}
This is what I meant... if we keep chan_id for paltform assigned IDs,
these filter functions could simply become
static bool pl330_filter(struct dma_chan *chan, void *param)
{
return chan->chan_id == param
}
And ideally in the long run, we could just drop the filter callback
and add expected channel ID to the request_channel call.
> +
> +static inline int s3c_dma_trigger(unsigned ch)
> +{
> + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
> +}
> +
> +static inline int s3c_dma_started(unsigned ch)
> +{
> + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
> +}
> +
> +static inline int s3c_dma_flush(unsigned ch)
> +{
> + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
> +}
> +
> +static inline int s3c_dma_stop(unsigned ch)
> +{
> + return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
> +}
> +
> +static struct samsung_dma_ops s3c_dma_ops = {
> + .request = s3c_dma_request,
> + .release = s3c_dma_release,
> + .prepare = s3c_dma_prepare,
> + .trigger = s3c_dma_trigger,
> + .started = s3c_dma_started,
> + .flush = s3c_dma_flush,
> + .stop = s3c_dma_stop,
These last 4 should be gnereallized into one callback with OP argument.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
Russell King - ARM Linux Wrote:
> Sent: Monday, July 25, 2011 6:36 PM
> To: Boojin Kim
> Cc: linux-arm-ker...@lists.infradead.org;
linux-samsung-soc@vger.kernel.org;
> Kukjin Kim; Vinod Koul; Jassi Brar; Grant Likely; Mark Brown; Dan Williams
> Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
>
> On Mon, Jul 25, 2011 at 10:28:24AM +0900, Boojin Kim wrote:
> > +static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
> > + struct samsung_dma_info *info)
> > +{
> > + struct dma_chan *chan;
> > + dma_cap_mask_t mask;
> > + struct dma_slave_config slave_config;
> > +
> > + dma_cap_zero(mask);
> > + dma_cap_set(info->cap, mask);
> > +
> > + chan = dma_request_channel(mask, pl330_filter, (void *)dma_ch);
> > +
> > + if (info->direction == DMA_FROM_DEVICE) {
> > + memset(&slave_config, 0, sizeof(struct dma_slave_config));
> > + slave_config.direction = info->direction;
> > + slave_config.src_addr = info->fifo;
> > + slave_config.src_addr_width = info->width;
>
> This should really set slave_config.src_maxburst to something sensible
too,
> even if that's just '1'.
I will address your comment.
>
> > + dmaengine_slave_config(chan, &slave_config);
> > + } else if (info->direction == DMA_TO_DEVICE) {
> > + memset(&slave_config, 0, sizeof(struct dma_slave_config));
> > + slave_config.direction = info->direction;
> > + slave_config.dst_addr = info->fifo;
> > + slave_config.dst_addr_width = info->width;
>
> Ditto for dst_maxburst.
>
> > + dmaengine_slave_config(chan, &slave_config);
> > + }
> > +
> > + return (unsigned)chan;
>
> I hope these interfaces yet cleaned away soon so that these casts can be
> killed off.
>
> > +struct samsung_dma_prep_info {
> > + enum dma_transaction_type cap;
> > + enum dma_data_direction direction;
> > + unsigned buf;
>
> dma_addr_t ?
>
> > + unsigned long period;
> > + unsigned long len;
> > + void (*fp)(void *data);
> > + void *fp_param;
> > +};
> > +
> > +struct samsung_dma_info {
> > + enum dma_transaction_type cap;
> > + enum dma_data_direction direction;
> > + enum dma_slave_buswidth width;
> > + unsigned fifo;
>
> dma_addr_t ?
>
> > + struct s3c2410_dma_client *client;
> > +};
> > +
> > +struct samsung_dma_ops {
> > + bool init;
>
> Unused?
Yes, It's my mistake. I will address your comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Mon, Jul 25, 2011 at 10:28:24AM +0900, Boojin Kim wrote:
> +static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
> + struct samsung_dma_info *info)
> +{
> + struct dma_chan *chan;
> + dma_cap_mask_t mask;
> + struct dma_slave_config slave_config;
> +
> + dma_cap_zero(mask);
> + dma_cap_set(info->cap, mask);
> +
> + chan = dma_request_channel(mask, pl330_filter, (void *)dma_ch);
> +
> + if (info->direction == DMA_FROM_DEVICE) {
> + memset(&slave_config, 0, sizeof(struct dma_slave_config));
> + slave_config.direction = info->direction;
> + slave_config.src_addr = info->fifo;
> + slave_config.src_addr_width = info->width;
This should really set slave_config.src_maxburst to something sensible too,
even if that's just '1'.
> + dmaengine_slave_config(chan, &slave_config);
> + } else if (info->direction == DMA_TO_DEVICE) {
> + memset(&slave_config, 0, sizeof(struct dma_slave_config));
> + slave_config.direction = info->direction;
> + slave_config.dst_addr = info->fifo;
> + slave_config.dst_addr_width = info->width;
Ditto for dst_maxburst.
> + dmaengine_slave_config(chan, &slave_config);
> + }
> +
> + return (unsigned)chan;
I hope these interfaces yet cleaned away soon so that these casts can be
killed off.
> +struct samsung_dma_prep_info {
> + enum dma_transaction_type cap;
> + enum dma_data_direction direction;
> + unsigned buf;
dma_addr_t ?
> + unsigned long period;
> + unsigned long len;
> + void (*fp)(void *data);
> + void *fp_param;
> +};
> +
> +struct samsung_dma_info {
> + enum dma_transaction_type cap;
> + enum dma_data_direction direction;
> + enum dma_slave_buswidth width;
> + unsigned fifo;
dma_addr_t ?
> + struct s3c2410_dma_client *client;
> +};
> +
> +struct samsung_dma_ops {
> + bool init;
Unused?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
This patch adds common DMA operations which are used for Samsung DMA
drivers. Currently there are two types of DMA driver for Samsung SoCs.
The one is S3C-DMA for S3C SoCs and the other is PL330-DMA for S5P SoCs.
This patch provides funcion pointers for common DMA operations to DMA
client driver like SPI and Audio. It makes DMA client drivers support
multi-platform.
In addition, this common DMA operations implement the shared actions
that are needed for DMA client driver. For example shared actions are
filter() function for dma_request_channel() and parameter passing for
device_prep_slave_sg().
Signed-off-by: Boojin Kim
---
arch/arm/mach-s3c2410/include/mach/dma.h |8 ++-
arch/arm/mach-s3c64xx/include/mach/dma.h |4 +
arch/arm/plat-samsung/Makefile |6 +-
arch/arm/plat-samsung/dma-ops.c| 133
arch/arm/plat-samsung/include/plat/dma-ops.h | 64 +++
arch/arm/plat-samsung/include/plat/dma-pl330.h |4 +
arch/arm/plat-samsung/include/plat/dma.h |1 +
arch/arm/plat-samsung/s3c-dma-ops.c| 133
8 files changed, 350 insertions(+), 3 deletions(-)
create mode 100644 arch/arm/plat-samsung/dma-ops.c
create mode 100644 arch/arm/plat-samsung/include/plat/dma-ops.h
create mode 100644 arch/arm/plat-samsung/s3c-dma-ops.c
diff --git a/arch/arm/mach-s3c2410/include/mach/dma.h
b/arch/arm/mach-s3c2410/include/mach/dma.h
index b2b2a5b..e61a91f 100644
--- a/arch/arm/mach-s3c2410/include/mach/dma.h
+++ b/arch/arm/mach-s3c2410/include/mach/dma.h
@@ -13,7 +13,6 @@
#ifndef __ASM_ARCH_DMA_H
#define __ASM_ARCH_DMA_H __FILE__
-#include
#include
#define MAX_DMA_TRANSFER_SIZE 0x10 /* Data Unit is half word */
@@ -51,6 +50,13 @@ enum dma_ch {
DMACH_MAX, /* the end entry */
};
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return false;
+}
+
+#include
+
#define DMACH_LOW_LEVEL(1<<28) /* use this to specifiy hardware ch no
*/
/* we have 4 dma channels */
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h
b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 0a5d926..49c3a53 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -63,6 +63,10 @@ static __inline__ bool s3c_dma_has_circular(void)
return true;
}
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return false;
+}
#define S3C2410_DMAF_CIRCULAR (1 << 0)
#include
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index 53eb15b..9ecf2aa 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -62,9 +62,11 @@ obj-$(CONFIG_SAMSUNG_DEV_PWM)+= dev-pwm.o
# DMA support
-obj-$(CONFIG_S3C_DMA) += dma.o
+obj-$(CONFIG_S3C_DMA) += dma.o s3c-dma-ops.o
-obj-$(CONFIG_S3C_PL330_DMA)+= s3c-pl330.o
+obj-$(CONFIG_DMADEV_PL330) += dma-ops.o
+
+obj-$(CONFIG_S3C_PL330_DMA)+= s3c-pl330.o s3c-dma-ops.o
# PM support
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
new file mode 100644
index 000..9053433
--- /dev/null
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -0,0 +1,133 @@
+/* linux/arch/arm/plat-samsung/dma-ops.c
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung DMA Operations
+ *
+ * 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.
+ */
+
+#include
+#include
+#include
+
+#include
+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+ struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan->private;
+ unsigned dma_ch = (unsigned)param;
+
+ if (peri->peri_id != dma_ch)
+ return false;
+
+ return true;
+}
+
+static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
+ struct samsung_dma_info *info)
+{
+ struct dma_chan *chan;
+ dma_cap_mask_t mask;
+ struct dma_slave_config slave_config;
+
+ dma_cap_zero(mask);
+ dma_cap_set(info->cap, mask);
+
+ chan = dma_request_channel(mask, pl330_filter, (void *)dma_ch);
+
+ if (info->direction == DMA_FROM_DEVICE) {
+ memset(&slave_config, 0, sizeof(struct dma_slave_config));
+ slave_config.direction = info->direction;
+ slave_config.src_addr = info->fifo;
+ slave_config.src_addr_width = info->width;
+ dmaengine_slave_config(chan, &slave_config);
+ } else if (info->direction == DMA_TO_DEVICE) {
+ memset(&slave_config, 0, sizeof(struct dma_slave_config));
+ slave_config.direction = info->direction;
+ slave_config.dst_addr = info->fifo;
+ slave_config.dst_addr_width = info->width;
+
