>-----Original Message-----
>From: Russell King - ARM Linux [mailto:[email protected]]
>Sent: Wednesday, September 24, 2014 8:04 PM
>To: Lothar Waßmann
>Cc: Lu Jingchang-B35083; [email protected]; [email protected]; linux-
>[email protected]; [email protected]; linux-arm-
>[email protected]
>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�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf��^jǫy�m��@A�a���
0��h���i
