RE: [PATCH] PPC4xx: ADMA separating SoC specific functions
> > > > You definitely need to be able to resolve "used but not defined" and > > "defined but not used" warnings before tackling a driver conversion > > like this. In light of this comment I wonder if it would be > > appropriate to submit your original driver, that just duplicated > > routines from the ppc440spe driver, to the -staging tree. Then it > > would be available for someone familiar with driver conversions to > > take a shot at unifying. > > > > Greg, is this an appropriate use of -staging? > > Possibly, but I really don't like duplication if possible. What's > keeping this code from being fixed up now properly? [Marri] Hello Greg, I am working on restructuring ppc4xx/adma.c driver into Common code and SoC specific code. This way I can add support for similar DMA engines. In this process I had to declare some Of the functions non static so that I can suppress "defined but not used" and "used but not defined". Here is what series of patches I planned to work on. 1. First set patches to re-arrange the code. Functionally no change except Structured into different files. 2. Second set to rename the common functions which are shared between SoC dma-engines. 3. Add support of new DMA engine for different SoC. I am working on first patch set right now. Regards, Marri ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] PPC4xx: ADMA separating SoC specific functions
On Thu, Sep 30, 2010 at 05:57:10PM -0700, Dan Williams wrote: > [ adding Greg ] > > On Thu, Sep 30, 2010 at 5:16 PM, Tirumala Marri wrote: > >> 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. > > This is part of defining a common interface. Maybe look at the > linkages of how the common ioat_probe() routine is used to support all > three versions of its dma hardware. > > > 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. > > You definitely need to be able to resolve "used but not defined" and > "defined but not used" warnings before tackling a driver conversion > like this. In light of this comment I wonder if it would be > appropriate to submit your original driver, that just duplicated > routines from the ppc440spe driver, to the -staging tree. Then it > would be available for someone familiar with driver conversions to > take a shot at unifying. > > Greg, is this an appropriate use of -staging? Possibly, but I really don't like duplication if possible. What's keeping this code from being fixed up now properly? thanks, greg k-h ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] PPC4xx: ADMA separating SoC specific functions
> You definitely need to be able to resolve "used but not defined" and > "defined but not used" warnings before tackling a driver conversion > like this. In light of this comment I wonder if it would be > appropriate to submit your original driver, that just duplicated > routines from the ppc440spe driver, to the -staging tree. Then it > would be available for someone familiar with driver conversions to > take a shot at unifying. > > Greg, is this an appropriate use of -staging? The other option is to define non static functions in ppc440spe-adma.c which are used in common File adma.c . This way there will not be any warnings. Is this something acceptable ? Here is the break down ppc440spe-adma.c: It will have all the 440spe SoC specific functions. ppc4xx_adma.h will have the declarations from 440spe-adma.c as non static. adma.c will have common functions which are independent of SoC. Please suggest. Regards, -Marri ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] PPC4xx: ADMA separating SoC specific functions
[ adding Greg ] On Thu, Sep 30, 2010 at 5:16 PM, Tirumala Marri wrote: >> 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. This is part of defining a common interface. Maybe look at the linkages of how the common ioat_probe() routine is used to support all three versions of its dma hardware. > 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. You definitely need to be able to resolve "used but not defined" and "defined but not used" warnings before tackling a driver conversion like this. In light of this comment I wonder if it would be appropriate to submit your original driver, that just duplicated routines from the ppc440spe driver, to the -staging tree. Then it would be available for someone familiar with driver conversions to take a shot at unifying. Greg, is this an appropriate use of -staging? -- Dan ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] PPC4xx: ADMA separating SoC specific functions
> On Thu, Sep 30, 2010 at 12:08 PM, Wolfgang Denk 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] PPC4xx: ADMA separating SoC specific functions
>
> When reposting a patch, please always indicate that this is new
> version by using something like "[PATCH v2]" in the Subject line.
[Marri] I know, but this patch is not modification of previous patch.
It is complete brand new from scratch again. In that case isn't this
will be first version ?
> > ---
>
> Also, please include here (i. e. below the "---" line, i. e. in the
> comments section, a description of what was changed compared to the
> previous version of this patch.
>
> As is, you enforce us to rescan the whole patch again and check
> manually if you have reacted to any of the comments sent before, and
> how. As is, you make reviewing your poatches harder than necessary.
[Marri} I will include comments in the further versions of this patch.
>
>
> > diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> > index 0d58a4a..a1053cb 100644
> > --- a/drivers/dma/ppc4xx/adma.c
> > +++ b/drivers/dma/ppc4xx/adma.c
> ...
> > +#include "ppc440spe-adma.h"
> > +
> > +struct dma_async_tx_descriptor
> > +*ppc440spe_adma_prep_dma_pq(struct dma_chan *chan,
> > + dma_addr_t * dst,
> > + dma_addr_t * src,
> > + unsigned int src_cnt,
> > + const unsigned char *scf,
> > + size_t len,
> > + unsigned long flags);
> > +struct dma_async_tx_descriptor
> > +*ppc440spe_adma_prep_dma_pqzero_sum(struct dma_chan *chan,
>
> Should such 440SPe specific code not be removed here and placed into
> ppc440spe-adma.c instead?
[Marri] It is 440SPe specific. Definition is moved ppc440spe-adma.c
>
> > +#if 0
> > static void prep_dma_pq_dbg(int id, dma_addr_t *dst, dma_addr_t
> *src,
> > unsigned int src_cnt)
> > {
> > @@ -213,8 +104,9 @@ static void prep_dma_pq_dbg(int id, dma_addr_t
> *dst, dma_addr_t *src,
> > for (i = 0; i < 2; i++)
> > pr_debug("\t0x%016llx ", dst[i]);
> > }
> > +#endif
>
> Please do not add dead code - remove the whole "#if 0" block.
[Marri] Will remove it.
> ***/
>
> It seems youremove all code, but leave the (now empty) comment
> headers? This makes little sense to me.
>
[Marri] Will clean up in the next version.
> ...
> > /**
> > * ppc440spe_adma_free_slots - flags descriptor slots for reuse
> > * @slot: Slot to free
> > * Caller must hold &ppc440spe_chan->lock while calling this
> function
> > */
>
> Again, all this is pretty low-level 440SPe specific code. Why do you
> keep this in the common drive rfile instead of moving it into the new
> 440SPe specific file?
[Marri]. With name change it can be used With any SoC ADMA driver.
>
>
> > diff --git a/drivers/dma/ppc4xx/ppc440spe-adma.c
> b/drivers/dma/ppc4xx/ppc440spe-adma.c
> > new file mode 100644
> > index 000..da467b4
> ...
> > + /* In the current implementation of ppc440spe ADMA driver it
> > +
> > +
> > +
> > +* makes sense to pick out only pq case, because it may be
>
> Formatting problems?
[Marri] Will fix in next version.
>
>
> > diff --git a/drivers/dma/ppc4xx/ppc440spe-adma.h
> b/drivers/dma/ppc4xx/ppc440spe-adma.h
> > new file mode 100644
> > index 000..81a1f46
> > --- /dev/null
> > +++ b/drivers/dma/ppc4xx/ppc440spe-adma.h
> ...
> > +/*
> > + * ppc440spe_get_group_entry - get group entry with index idx
> > + * @tdesc: is the last allocated slot in the group.
> > + */
> > +static struct ppc440spe_adma_desc_slot
> *ppc440spe_get_group_entry(struct
> > +
ppc440spe_adma_desc_slot
> > + *tdesc,
> > + u32 entry_idx)
> > +{
> > + struct ppc440spe_adma_desc_slot *iter = tdesc->group_head;
> > + int i = 0;
> > +
> > + if (entry_idx < 0 || entry_idx >= (tdesc->src_cnt + tdesc-
> >dst_cnt)) {
> > + printk("%s: entry_idx %d, src_cnt %d, dst_cnt %d\n",
> > + __func__, entry_idx, tdesc->src_cnt, tdesc-
> >dst_cnt);
> > + BUG();
> > + }
> > +
> > + list_for_each_entry(iter, &tdesc->group_list, chain_node) {
> > + if (i++ == entry_idx)
> > + break;
> > + }
> > + return iter;
> > +}
>
> This is a header file, yet you add here literally thousands of lines of
> code.
>
>
> Note that more or less similar questions have been asked for the
> previous version of this patch, but I fail to find any good
> justification in your replies.
[Marri] Reason for some functions in lined is 1) They are small enough
To be in lined 2) If keep them in ppc440spe-adma.c I will have to make
them
Non static to avoid "Used but not defined warnings". With too many
functions
Non static might cause name space pollution in the kernel ?
>
>
> 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 pre
Re: [PATCH] PPC4xx: ADMA separating SoC specific functions
On Thu, Sep 30, 2010 at 12:08 PM, Wolfgang Denk 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.
> 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.
--
Dan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] PPC4xx: ADMA separating SoC specific functions
Dear tma...@apm.com,
In message <1285865736-32074-1-git-send-email-tma...@apm.com> you wrote:
> From: Tirumala Marri
>
> This patch separates the SoC specific functions and moved
> to different files.
>
> The reason for ppc440spe-adma.h is to define in-line functions which
> are called by both adma.c and ppc440spe-adma.c .
>
> Where as ppc440spe-adma.c is to define functions are completely
> completely dependent on 440spe, also which are too big to define
> as in-line functions.
When reposting a patch, please always indicate that this is new
version by using something like "[PATCH v2]" in the Subject line.
> Signed-off-by: Tirumala R Marri
> Acked-by: Yuri Tikhonov
> CC: Dan Williams
> CC: Josh Boyer
> ---
Also, please include here (i. e. below the "---" line, i. e. in the
comments section, a description of what was changed compared to the
previous version of this patch.
As is, you enforce us to rescan the whole patch again and check
manually if you have reacted to any of the comments sent before, and
how. As is, you make reviewing your poatches harder than necessary.
> diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
> index 0d58a4a..a1053cb 100644
> --- a/drivers/dma/ppc4xx/adma.c
> +++ b/drivers/dma/ppc4xx/adma.c
...
> +#include "ppc440spe-adma.h"
> +
> +struct dma_async_tx_descriptor
> +*ppc440spe_adma_prep_dma_pq(struct dma_chan *chan,
> +dma_addr_t * dst,
> +dma_addr_t * src,
> +unsigned int src_cnt,
> +const unsigned char *scf,
> +size_t len,
> +unsigned long flags);
> +struct dma_async_tx_descriptor
> +*ppc440spe_adma_prep_dma_pqzero_sum(struct dma_chan *chan,
Should such 440SPe specific code not be removed here and placed into
ppc440spe-adma.c instead?
> +#if 0
> static void prep_dma_pq_dbg(int id, dma_addr_t *dst, dma_addr_t *src,
> unsigned int src_cnt)
> {
> @@ -213,8 +104,9 @@ static void prep_dma_pq_dbg(int id, dma_addr_t *dst,
> dma_addr_t *src,
> for (i = 0; i < 2; i++)
> pr_debug("\t0x%016llx ", dst[i]);
> }
> +#endif
Please do not add dead code - remove the whole "#if 0" block.
>
> /**
> * ADMA channel low-level routines
>
> **/
>
> -static u32
...
...
> -}
>
>
> /**
> * ADMA device level
>
> **/
It seems youremove all code, but leave the (now empty) comment
headers? This makes little sense to me.
...
> /**
> * ppc440spe_adma_free_slots - flags descriptor slots for reuse
> * @slot: Slot to free
> * Caller must hold &ppc440spe_chan->lock while calling this function
> */
Again, all this is pretty low-level 440SPe specific code. Why do you
keep this in the common drive rfile instead of moving it into the new
440SPe specific file?
> diff --git a/drivers/dma/ppc4xx/ppc440spe-adma.c
> b/drivers/dma/ppc4xx/ppc440spe-adma.c
> new file mode 100644
> index 000..da467b4
...
> + /* In the current implementation of ppc440spe ADMA driver it
> +
> +
> +
> + * makes sense to pick out only pq case, because it may be
Formatting problems?
> diff --git a/drivers/dma/ppc4xx/ppc440spe-adma.h
> b/drivers/dma/ppc4xx/ppc440spe-adma.h
> new file mode 100644
> index 000..81a1f46
> --- /dev/null
> +++ b/drivers/dma/ppc4xx/ppc440spe-adma.h
...
> +/*
> + * ppc440spe_get_group_entry - get group entry with index idx
> + * @tdesc: is the last allocated slot in the group.
> + */
> +static struct ppc440spe_adma_desc_slot *ppc440spe_get_group_entry(struct
> +
> ppc440spe_adma_desc_slot
> + *tdesc,
> + u32 entry_idx)
> +{
> + struct ppc440spe_adma_desc_slot *iter = tdesc->group_head;
> + int i = 0;
> +
> + if (entry_idx < 0 || entry_idx >= (tdesc->src_cnt + tdesc->dst_cnt)) {
> + printk("%s: entry_idx %d, src_cnt %d, dst_cnt %d\n",
> +__func__, entry_idx, tdesc->src_cnt, tdesc->dst_cnt);
> + BUG();
> + }
> +
> + list_for_each_entry(iter, &tdesc->group_list, chain_node) {
> + if (i++ == entry_idx)
> + break;
> + }
> + return iter;
> +}
This is a header file, yet you add here literally thousands of lines of
code.
Note that more or less similar questions have been asked for the
previous version of this patch, but I fail to find any good
justification in your replies.
Selecting the architecture at build time is bad as it prevents using a
sinlge kern
