[PATCH] PPC4xx: ADMA separating SoC specific functions

Tirumala Marri tmarri at apm.com
Fri Oct 1 10:16:30 EST 2010


> On Thu, Sep 30, 2010 at 12:08 PM, Wolfgang Denk <wd at denx.de> wrote:
> [snip other valid review comments]
> >
> > This is a header file, yet you add here literally thousands of lines
> of
> > code.
>
> Yes, these functions are entirely too large to be inlined.  It looks
> like you are trying to borrow too heavily from the iop-adma model.
> The differences between iop13xx and iop33x from a adma perspective are
> just in descriptor format and channel capabilities.  If you look at
> the routines implemented in:
> arch/arm/include/asm/hardware/iop3xx-adma.h
> arch/arm/mach-iop13xx/include/mach/adma.h
> ...they are just simple helpers that abstract the descriptor details.
> For example:
>
> iop_adma_prep_dma_xor()
> {
> [snip]
>         spin_lock_bh(&iop_chan->lock);
>         slot_cnt = iop_chan_xor_slot_count(len, src_cnt,
> &slots_per_op);
>         sw_desc = iop_adma_alloc_slots(iop_chan, slot_cnt,
> slots_per_op);
>         if (sw_desc) {
>                 grp_start = sw_desc->group_head;
>                 iop_desc_init_xor(grp_start, src_cnt, flags);
>                 iop_desc_set_byte_count(grp_start, iop_chan, len);
>                 iop_desc_set_dest_addr(grp_start, iop_chan, dma_dest);
>                 sw_desc->unmap_src_cnt = src_cnt;
>                 sw_desc->unmap_len = len;
>                 sw_desc->async_tx.flags = flags;
>                 while (src_cnt--)
>                         iop_desc_set_xor_src_addr(grp_start, src_cnt,
>                                                   dma_src[src_cnt]);
>         }
>         spin_unlock_bh(&iop_chan->lock);
>
>         return sw_desc ? &sw_desc->async_tx : NULL;
> }
>
> Where  iop_adma_alloc_slots() is implemented differently between
> iop13xx and iop3xx.  In this case why does ppc440spe-adma.h exist?  If
> it has code specific to ppe440spe it should just live in the ppe440spe
> C file.  If it is truly generic it should move to the base adma.c
> implementation.  If you want to reuse a ppe440spe routine just link to
> it.
[Marri]That is how I started changing the code. And I see tons of warnings
Saying "Used but not defined" or "Defined but not used". How should I
suppress
Some functions from adma.c are used in ppc440spe-adma.c and some from
ppc440spe-adma.c
Are used in adma.c. So I created intermediate file ppc440spe-adma.h with
inlined
Functions. In future this will be converted into ppc4xx_adma.h and move
existing
SoC specific stuff to ppc440spe-adma.c file.

>
> > Selecting the architecture at build time is bad as it prevents using
> a
> > sinlge kernel image across a wide range of boards.  You only replied
> > "We select the architecture at build time." without any explanation
> if
> > there is a pressing technical reason to do it this way, or if this
> was
> > just a arbitrary decision.
>
> I agree I have yet to see any indication that this driver needs to
> have an architecture selected at build time.
>
> A potential compromise a first step would be to have a common C file
> that is shared between two driver modules until such point that they
> can be unified into a common module.

As I responded to Mr. Wolfgang in previous email, similar SoC DMA engines
will
Be resolved at run time. Whereas completely different architectures will
be
Resolved at build time.

Regards,
Marri


More information about the Linuxppc-dev mailing list