RE: [PATCH] PPC4xx: ADMA separating SoC specific functions

[Tirumala Marri <tmarri@apm.com>]

> On Thu, Sep 30, 2010 at 12:08 PM, Wolfgang Denk <wd@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 =3D iop_chan_xor_slot_count(len, src_cnt,
> &slots_per_op);
>         sw_desc =3D iop_adma_alloc_slots(iop_chan, slot_cnt,
> slots_per_op);
>         if (sw_desc) {
>                 grp_start =3D 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 =3D src_cnt;
>                 sw_desc->unmap_len =3D len;
>                 sw_desc->async_tx.flags =3D 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. =A0You 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