RE: [PATCHv3] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Jingchang Lu]

>-----Original Message-----
>From: Russell King - ARM Linux [mailto:linux@xxxxxxxxxxxxxxxx]
>Sent: Wednesday, September 24, 2014 8:04 PM
>To: Lothar WaÃmann
>Cc: Lu Jingchang-B35083; vinod.koul@xxxxxxxxx; arnd@xxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [PATCHv3] dmaengine: fsl-edma: fixup reg offset and hw S/G
>support in big-endian model
>
>On Wed, Sep 24, 2014 at 01:05:10PM +0200, Lothar WaÃmann wrote:
>> Hi,
>>
>> Jingchang Lu wrote:
>> > +	 * eDMA hardware SGs requires the TCDs to be auto loaded
>> > +	 * in the little endian whenver the register endian model,
>> "in little endian whatever the register endian model"
>
>Even that took several reads to work it out.
>
>	eDMA hardware SGs require the TCDs to be stored in little endian
>	format irrespective of the register endian model.
>
>and I think that's all that needs to be said here.
>
>However, as I realdy suggested, running this ruddy thing through sparse
>would be a /very/ good idea, because it'll warn with:
>
>+       tcd->daddr = cpu_to_le32(dst);
>
>The reason it'll warn on that is because daddr is not declared with __le32,
>etc - the types used in struct fsl_edma_hw_tcd tell sparse that the values
>to be stored there are in _host_ endian format, but they're being assigned
>little endian formatted data.
>
I didn't realize the type warning, they indeed exist. I will use __le32 and
__le16 for fsl_edma_hw_tcd struct members as below:
struct fsl_edma_hw_tcd {
        __le32  saddr;
        __le16  soff;
        __le16  attr;
        __le32  nbytes;
        __le32  slast;
        __le32  daddr;
        __le16  doff;
        __le16  citer;
        __le32  dlast_sga;
        __le16  csr;
        __le16  biter;
};

>> > +	 * for fsl_set_tcd_params doing the swap.
>> fsl_set_tcd_params()
>
>That's the wrong function name anyway.  It's fsl_edma_set_tcd_params().
>
>However, let's look at this a little more:
>
>        fsl_edma_set_tcd_params(fsl_chan, tcd->saddr, tcd->daddr, tcd-
>>attr,
>                        tcd->soff, tcd->nbytes, tcd->slast, tcd->citer,
>                        tcd->biter, tcd->doff, tcd->dlast_sga, tcd->csr);
>
>Is it /really/ a good idea to read all that data out of the structure,
>only to then have most of it saved into the stack, which
>fsl_edma_set_tcd_params() then has to read back off the stack again?
>With stuff like this, one has to wonder if there is much clue how to write
>optimal C code in this driver.
>
>This should be passing the tcd structure into fsl_edma_set_tcd_params().
>
>Now, this function contains this comment:
>
>        /*
>         * TCD parameters have been swapped in fill_tcd_params(),
>         * so just write them to registers in the cpu endian here
>         */
>
>Which is almost reasonable, but let's update it:
>
>	TCD parameters are stored in struct fsl_edma_hw_tcd in little
>	endian format.  However, we need to load the registers in
>	<insert appropriate endianness - big|little|cpu> endian.
>
>Now, remember that writel() and friends expect native CPU endian formatted
>data (and yes, sparse checks this too) so that needs to be considered more.
>
>Lastly, the driver seems to be a total mess of accessors.  In some places
>it uses io{read,write}*, in other places, it uses plain {read,write}*.  It
>should use either one set or the other set, and not mix these two.
>
>I just spotted this badly formatted code too:
>
>        for (i = 0; i < fsl_desc->n_tcds; i++)
>                        dma_pool_free(fsl_desc->echan->tcd_pool,
>                                        fsl_desc->tcd[i].vtcd,
>                                        fsl_desc->tcd[i].ptcd); ...
>                edma_writeb(fsl_chan->edma,
>                                EDMAMUX_CHCFG_ENBL |
>EDMAMUX_CHCFG_SOURCE(slot),
>                                muxaddr + ch_off);
>
>--
Thanks, I will use the tcd pointer instead. And I will use one r/w set.

Best Regards,
Jingchang
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå