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

From: Jingchang Lu
Date: Thu Sep 25 2014 - 03:39:27 EST

>-----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-
>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-
> 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,
> muxaddr + ch_off);
Thanks, I will use the tcd pointer instead. And I will use one r/w set.

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