RE: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support
Hi, Rob, Could you please help review the devicetree binding? Thanks! Best Regards, Jingchang > -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Thursday, October 10, 2013 11:44 PM > To: Lu Jingchang-B35083 > Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; devicet...@vger.kernel.org > Subject: Re: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support > > On Thu, Oct 10, 2013 at 09:39:04AM +, Lu Jingchang-B35083 wrote: > > Hi, Vinod, > > > > Could you please help review this patch? Could it be merged into your > next tree? > > Thanks! > Where is the patch 1 & 2 of the series, I dont sem to find it. > Also I need ACK from one of the DT maintainers on the binding before I > can carry > this > > -- > ~Vinod > > > > > > Best Regards, > > Jingchang > > > > > > > -Original Message- > > > From: Lu Jingchang-B35083 > > > Sent: Wednesday, September 18, 2013 5:58 PM > > > To: vinod.k...@intel.com > > > Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- > > > ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu Jingchang- > > > B35083; Wang Huan-B18965 > > > Subject: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support > > > > > > Add Freescale enhanced direct memory(eDMA) controller support. > > > The eDMA controller deploys DMAMUXs routing DMA request sources(slot) > > > to eDMA channels. > > > This module can be found on Vybrid and LS-1 SoCs. > > > > > > Signed-off-by: Alison Wang > > > Signed-off-by: Jingchang Lu > > > --- > > > changes in v6: > > > holding lock in dma_control to prevent race. > > > > > > changes in v5: > > > config slave_id when dmaengine_slave_config intead of channel > request. > > > adding residue calculation and dma pause/resume device control. > > > > > > changes in v4: > > > using exact compatible string in binding document. > > > > > > changes in v3: > > > handle all pending interrupt one time. > > > add protect lock on dma transfer complete handling. > > > change desc and tcd alloc flag to GFP_NOWAIT. > > > add sanity check and error messages. > > > > > > changes in v2: > > > using generic dma-channels property instead of fsl,dma-channel. > > > rename the binding document to fsl-edma.txt. > > > > > > > > > Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++ > > > drivers/dma/Kconfig| 10 + > > > drivers/dma/Makefile | 1 + > > > drivers/dma/fsl-edma.c | 944 > > > + > > > 4 files changed, 1039 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/dma/fsl- > edma.txt > > > create mode 100644 drivers/dma/fsl-edma.c > > > > --
RE: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support
Hi, Rob, Could you please help review the devicetree binding? Thanks! Best Regards, Jingchang -Original Message- From: Vinod Koul [mailto:vinod.k...@intel.com] Sent: Thursday, October 10, 2013 11:44 PM To: Lu Jingchang-B35083 Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- ker...@lists.infradead.org; devicet...@vger.kernel.org Subject: Re: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support On Thu, Oct 10, 2013 at 09:39:04AM +, Lu Jingchang-B35083 wrote: Hi, Vinod, Could you please help review this patch? Could it be merged into your next tree? Thanks! Where is the patch 1 2 of the series, I dont sem to find it. Also I need ACK from one of the DT maintainers on the binding before I can carry this -- ~Vinod Best Regards, Jingchang -Original Message- From: Lu Jingchang-B35083 Sent: Wednesday, September 18, 2013 5:58 PM To: vinod.k...@intel.com Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu Jingchang- B35083; Wang Huan-B18965 Subject: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support Add Freescale enhanced direct memory(eDMA) controller support. The eDMA controller deploys DMAMUXs routing DMA request sources(slot) to eDMA channels. This module can be found on Vybrid and LS-1 SoCs. Signed-off-by: Alison Wang b18...@freescale.com Signed-off-by: Jingchang Lu b35...@freescale.com --- changes in v6: holding lock in dma_control to prevent race. changes in v5: config slave_id when dmaengine_slave_config intead of channel request. adding residue calculation and dma pause/resume device control. changes in v4: using exact compatible string in binding document. changes in v3: handle all pending interrupt one time. add protect lock on dma transfer complete handling. change desc and tcd alloc flag to GFP_NOWAIT. add sanity check and error messages. changes in v2: using generic dma-channels property instead of fsl,dma-channel. rename the binding document to fsl-edma.txt. Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++ drivers/dma/Kconfig| 10 + drivers/dma/Makefile | 1 + drivers/dma/fsl-edma.c | 944 + 4 files changed, 1039 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/fsl- edma.txt create mode 100644 drivers/dma/fsl-edma.c --
RE: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod, Could you please help review this patch? Could it be merged into your next tree? Thanks! Best Regards, Jingchang > -Original Message- > From: Lu Jingchang-B35083 > Sent: Wednesday, September 18, 2013 5:58 PM > To: vinod.k...@intel.com > Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu Jingchang- > B35083; Wang Huan-B18965 > Subject: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support > > Add Freescale enhanced direct memory(eDMA) controller support. > The eDMA controller deploys DMAMUXs routing DMA request sources(slot) > to eDMA channels. > This module can be found on Vybrid and LS-1 SoCs. > > Signed-off-by: Alison Wang > Signed-off-by: Jingchang Lu > --- > changes in v6: > holding lock in dma_control to prevent race. > > changes in v5: > config slave_id when dmaengine_slave_config intead of channel request. > adding residue calculation and dma pause/resume device control. > > changes in v4: > using exact compatible string in binding document. > > changes in v3: > handle all pending interrupt one time. > add protect lock on dma transfer complete handling. > change desc and tcd alloc flag to GFP_NOWAIT. > add sanity check and error messages. > > changes in v2: > using generic dma-channels property instead of fsl,dma-channel. > rename the binding document to fsl-edma.txt. > > > Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++ > drivers/dma/Kconfig| 10 + > drivers/dma/Makefile | 1 + > drivers/dma/fsl-edma.c | 944 > + > 4 files changed, 1039 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt > create mode 100644 drivers/dma/fsl-edma.c
RE: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod, Could you please help review this patch? Could it be merged into your next tree? Thanks! Best Regards, Jingchang -Original Message- From: Lu Jingchang-B35083 Sent: Wednesday, September 18, 2013 5:58 PM To: vinod.k...@intel.com Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu Jingchang- B35083; Wang Huan-B18965 Subject: [PATCH v6 3/3] dma: Add Freescale eDMA engine driver support Add Freescale enhanced direct memory(eDMA) controller support. The eDMA controller deploys DMAMUXs routing DMA request sources(slot) to eDMA channels. This module can be found on Vybrid and LS-1 SoCs. Signed-off-by: Alison Wang b18...@freescale.com Signed-off-by: Jingchang Lu b35...@freescale.com --- changes in v6: holding lock in dma_control to prevent race. changes in v5: config slave_id when dmaengine_slave_config intead of channel request. adding residue calculation and dma pause/resume device control. changes in v4: using exact compatible string in binding document. changes in v3: handle all pending interrupt one time. add protect lock on dma transfer complete handling. change desc and tcd alloc flag to GFP_NOWAIT. add sanity check and error messages. changes in v2: using generic dma-channels property instead of fsl,dma-channel. rename the binding document to fsl-edma.txt. Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++ drivers/dma/Kconfig| 10 + drivers/dma/Makefile | 1 + drivers/dma/fsl-edma.c | 944 + 4 files changed, 1039 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt create mode 100644 drivers/dma/fsl-edma.c
RE: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Monday, September 23, 2013 12:26 PM > To: Lu Jingchang-B35083 > Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org > Subject: Re: [PATCH 3/3] dma: Add Freescale eDMA engine driver support > > On Tue, Sep 17, 2013 at 08:08:46AM +, Lu Jingchang-B35083 wrote: > > > > + case DMA_PAUSE: > > > > + if (fsl_chan->edesc) > > > > + fsl_edma_disable_request(fsl_chan); > > > racy here too... > > It only set the channel HW register, no list is handled, > > is lock needed here? Thanks! > well thats the point while you are terminating the current trasnaction > can > complete and then start another one. You want to try and prevent these > case > also. Here you are neither locking the HW access nor the the list, so its > juts > waiting to crash! Thanks! I have sent out the new v6 patch last week with holding lock here, Could you please help review it? Best Regards, Jingchang
RE: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
-Original Message- From: Vinod Koul [mailto:vinod.k...@intel.com] Sent: Monday, September 23, 2013 12:26 PM To: Lu Jingchang-B35083 Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org Subject: Re: [PATCH 3/3] dma: Add Freescale eDMA engine driver support On Tue, Sep 17, 2013 at 08:08:46AM +, Lu Jingchang-B35083 wrote: + case DMA_PAUSE: + if (fsl_chan-edesc) + fsl_edma_disable_request(fsl_chan); racy here too... It only set the channel HW register, no list is handled, is lock needed here? Thanks! well thats the point while you are terminating the current trasnaction can complete and then start another one. You want to try and prevent these case also. Here you are neither locking the HW access nor the the list, so its juts waiting to crash! Thanks! I have sent out the new v6 patch last week with holding lock here, Could you please help review it? Best Regards, Jingchang
RE: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Tuesday, September 17, 2013 12:54 PM > To: Lu Jingchang-B35083 > Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Wang > Huan-B18965 > Subject: Re: [PATCH 3/3] dma: Add Freescale eDMA engine driver support > > + case DMA_PAUSE: > > + if (fsl_chan->edesc) > > + fsl_edma_disable_request(fsl_chan); > racy here too... It only set the channel HW register, no list is handled, is lock needed here? Thanks! Best Regards, Jingchang
RE: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Tuesday, September 17, 2013 12:54 PM
> To: Lu Jingchang-B35083
> Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Wang
> Huan-B18965
> Subject: Re: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
>
> I need ACK from DT maintainers on the above parts
Should I cc this to one of the DT maintainers? Thanks.
>
> > +static int fsl_edma_control(struct dma_chan *chan, enum dma_ctrl_cmd
> cmd,
> > + unsigned long arg)
> > +{
> > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > + struct dma_slave_config *cfg = (void *)arg;
> > +
> > + switch (cmd) {
> > + case DMA_TERMINATE_ALL:
> > + fsl_edma_disable_request(fsl_chan);
> > + fsl_chan->edesc = NULL;
> you need to hold the lock here to prevent race here!
Thanks, I will fix this in the next version of the patch.
>
> > + vchan_free_chan_resources(_chan->vchan);
> > + return 0;
> > +
> > + case DMA_SLAVE_CONFIG:
> > + fsl_chan->fsc.dir = cfg->direction;
> > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > + fsl_chan->fsc.dev_addr = cfg->src_addr;
> > + fsl_chan->fsc.addr_width = cfg->src_addr_width;
> > + fsl_chan->fsc.burst = cfg->src_maxburst;
> > + fsl_chan->fsc.attr = fsl_edma_get_tcd_attr(cfg-
> >src_addr_width);
> > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > + fsl_chan->fsc.dev_addr = cfg->dst_addr;
> > + fsl_chan->fsc.addr_width = cfg->dst_addr_width;
> > + fsl_chan->fsc.burst = cfg->dst_maxburst;
> > + fsl_chan->fsc.attr = fsl_edma_get_tcd_attr(cfg-
> >dst_addr_width);
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + if (!cfg->slave_id) {
> > + return -EINVAL;
> should you check for this first, you wrote the channel with config above
> and now
> returning error?
Yes, the slave id should be checked first, thanks!
> > + spin_lock_irqsave(_chan->vchan.lock, flags);
> > + vdesc = vchan_find_desc(_chan->vchan, cookie);
> > + if (fsl_chan->edesc && cookie == fsl_chan->edesc->vdesc.tx.cookie)
> > + txstate->residue = fsl_edma_desc_residue(fsl_chan, vdesc, 1);
> why not use true/false for the last bool arg?
Thanks, I will use true/false instead.
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Tuesday, September 17, 2013 12:54 PM
To: Lu Jingchang-B35083
Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Wang
Huan-B18965
Subject: Re: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
I need ACK from DT maintainers on the above parts
Should I cc this to one of the DT maintainers? Thanks.
+static int fsl_edma_control(struct dma_chan *chan, enum dma_ctrl_cmd
cmd,
+ unsigned long arg)
+{
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+ struct dma_slave_config *cfg = (void *)arg;
+
+ switch (cmd) {
+ case DMA_TERMINATE_ALL:
+ fsl_edma_disable_request(fsl_chan);
+ fsl_chan-edesc = NULL;
you need to hold the lock here to prevent race here!
Thanks, I will fix this in the next version of the patch.
+ vchan_free_chan_resources(fsl_chan-vchan);
+ return 0;
+
+ case DMA_SLAVE_CONFIG:
+ fsl_chan-fsc.dir = cfg-direction;
+ if (cfg-direction == DMA_DEV_TO_MEM) {
+ fsl_chan-fsc.dev_addr = cfg-src_addr;
+ fsl_chan-fsc.addr_width = cfg-src_addr_width;
+ fsl_chan-fsc.burst = cfg-src_maxburst;
+ fsl_chan-fsc.attr = fsl_edma_get_tcd_attr(cfg-
src_addr_width);
+ } else if (cfg-direction == DMA_MEM_TO_DEV) {
+ fsl_chan-fsc.dev_addr = cfg-dst_addr;
+ fsl_chan-fsc.addr_width = cfg-dst_addr_width;
+ fsl_chan-fsc.burst = cfg-dst_maxburst;
+ fsl_chan-fsc.attr = fsl_edma_get_tcd_attr(cfg-
dst_addr_width);
+ } else {
+ return -EINVAL;
+ }
+
+ if (!cfg-slave_id) {
+ return -EINVAL;
should you check for this first, you wrote the channel with config above
and now
returning error?
Yes, the slave id should be checked first, thanks!
+ spin_lock_irqsave(fsl_chan-vchan.lock, flags);
+ vdesc = vchan_find_desc(fsl_chan-vchan, cookie);
+ if (fsl_chan-edesc cookie == fsl_chan-edesc-vdesc.tx.cookie)
+ txstate-residue = fsl_edma_desc_residue(fsl_chan, vdesc, 1);
why not use true/false for the last bool arg?
Thanks, I will use true/false instead.
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 3/3] dma: Add Freescale eDMA engine driver support
-Original Message- From: Vinod Koul [mailto:vinod.k...@intel.com] Sent: Tuesday, September 17, 2013 12:54 PM To: Lu Jingchang-B35083 Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Wang Huan-B18965 Subject: Re: [PATCH 3/3] dma: Add Freescale eDMA engine driver support + case DMA_PAUSE: + if (fsl_chan-edesc) + fsl_edma_disable_request(fsl_chan); racy here too... It only set the channel HW register, no list is handled, is lock needed here? Thanks! Best Regards, Jingchang
RE: [PATCH v5 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod,
Could you please help review this patch?
Thanks!
Best Regards,
Jingchang
> -Original Message-
> From: Lu Jingchang-B35083
> Sent: Thursday, September 05, 2013 5:55 PM
> To: vinod.k...@intel.com
> Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu
> Jingchang-B35083; Wang Huan-B18965
> Subject: [PATCH v5 3/3] dma: Add Freescale eDMA engine driver support
>
> Add Freescale enhanced direct memory(eDMA) controller support.
> The eDMA controller deploys DMAMUXs routing DMA request sources(slot)
> to eDMA channels.
> This module can be found on Vybrid and LS-1 SoCs.
>
> Signed-off-by: Alison Wang
> Signed-off-by: Jingchang Lu
> ---
> changes in v5:
> config slave_id when dmaengine_slave_config intead of channel request.
> adding residue calculation and dma pause/resume device control.
>
> changes in v4:
> using exact compatible string in binding document.
>
> changes in v3:
> handle all pending interrupt one time.
> add protect lock on dma transfer complete handling.
> change desc and tcd alloc flag to GFP_NOWAIT.
> add sanity check and error messages.
>
> changes in v2:
> using generic dma-channels property instead of fsl,dma-channel.
> rename the binding document to fsl-edma.txt.
>
> Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++
> drivers/dma/Kconfig| 10 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-edma.c | 925
> +
> 4 files changed, 1020 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
> create mode 100644 drivers/dma/fsl-edma.c
>
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v5 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod,
Could you please help review this patch?
Thanks!
Best Regards,
Jingchang
-Original Message-
From: Lu Jingchang-B35083
Sent: Thursday, September 05, 2013 5:55 PM
To: vinod.k...@intel.com
Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu
Jingchang-B35083; Wang Huan-B18965
Subject: [PATCH v5 3/3] dma: Add Freescale eDMA engine driver support
Add Freescale enhanced direct memory(eDMA) controller support.
The eDMA controller deploys DMAMUXs routing DMA request sources(slot)
to eDMA channels.
This module can be found on Vybrid and LS-1 SoCs.
Signed-off-by: Alison Wang b18...@freescale.com
Signed-off-by: Jingchang Lu b35...@freescale.com
---
changes in v5:
config slave_id when dmaengine_slave_config intead of channel request.
adding residue calculation and dma pause/resume device control.
changes in v4:
using exact compatible string in binding document.
changes in v3:
handle all pending interrupt one time.
add protect lock on dma transfer complete handling.
change desc and tcd alloc flag to GFP_NOWAIT.
add sanity check and error messages.
changes in v2:
using generic dma-channels property instead of fsl,dma-channel.
rename the binding document to fsl-edma.txt.
Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++
drivers/dma/Kconfig| 10 +
drivers/dma/Makefile | 1 +
drivers/dma/fsl-edma.c | 925
+
4 files changed, 1020 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
create mode 100644 drivers/dma/fsl-edma.c
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Tuesday, September 03, 2013 7:32 PM
> To: Lu Jingchang-B35083
> Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
>
> On Tue, Sep 03, 2013 at 05:43:21AM +, Lu Jingchang-B35083 wrote:
> > Do you mean the DMA_SLAVE_CONFIG device_control? Yeah, the slave
> driver could pass
> > the slave_id. But the DMA_SLAVE_CONFIG may be called more than once,
> and the eDMA
> > driver just needs to set the slave id once for any given channel, after
> that the
> > transfer is transparent to the device.
> It depends, for a channel requested, if you are only tranferring to a
> particular
> slave device then it can be confugured once.
> so
> 1. allocate channel
> 2. dmaengine_slave_config()
>
> then you cnan do preare etc multiple times based on need.
But if the preferred slave id configuration should be in
dmaengine_slave_config,
I will send out this change in the next version patch.
Thanks!
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Tuesday, September 03, 2013 7:32 PM
To: Lu Jingchang-B35083
Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; devicet...@vger.kernel.org
Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
On Tue, Sep 03, 2013 at 05:43:21AM +, Lu Jingchang-B35083 wrote:
Do you mean the DMA_SLAVE_CONFIG device_control? Yeah, the slave
driver could pass
the slave_id. But the DMA_SLAVE_CONFIG may be called more than once,
and the eDMA
driver just needs to set the slave id once for any given channel, after
that the
transfer is transparent to the device.
It depends, for a channel requested, if you are only tranferring to a
particular
slave device then it can be confugured once.
so
1. allocate channel
2. dmaengine_slave_config()
then you cnan do preare etc multiple times based on need.
But if the preferred slave id configuration should be in
dmaengine_slave_config,
I will send out this change in the next version patch.
Thanks!
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message- > From: Vinod Koul [mailto:vinod.k...@intel.com] > Sent: Tuesday, September 03, 2013 7:32 PM > To: Lu Jingchang-B35083 > Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; devicet...@vger.kernel.org > Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support > > On Tue, Sep 03, 2013 at 05:43:21AM +, Lu Jingchang-B35083 wrote: > > Do you mean the DMA_SLAVE_CONFIG device_control? Yeah, the slave > driver could pass > > the slave_id. But the DMA_SLAVE_CONFIG may be called more than once, > and the eDMA > > driver just needs to set the slave id once for any given channel, after > that the > > transfer is transparent to the device. > It depends, for a channel requested, if you are only tranferring to a > particular > slave device then it can be confugured once. > so > 1. allocate channel > 2. dmaengine_slave_config() > > then you cnan do preare etc multiple times based on need. Yeah, I think we should just configure the slave id once and this would make the configuration simply. But by debugging, I find the dmaengine_slave_config() may be called more one times after requested by on user. So I put the slave id configuration in the channel request phase. And the effect is the same for exclusive channel. Thanks! Best Regards, Jingchang
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
-Original Message- From: Vinod Koul [mailto:vinod.k...@intel.com] Sent: Tuesday, September 03, 2013 7:32 PM To: Lu Jingchang-B35083 Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm- ker...@lists.infradead.org; devicet...@vger.kernel.org Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support On Tue, Sep 03, 2013 at 05:43:21AM +, Lu Jingchang-B35083 wrote: Do you mean the DMA_SLAVE_CONFIG device_control? Yeah, the slave driver could pass the slave_id. But the DMA_SLAVE_CONFIG may be called more than once, and the eDMA driver just needs to set the slave id once for any given channel, after that the transfer is transparent to the device. It depends, for a channel requested, if you are only tranferring to a particular slave device then it can be confugured once. so 1. allocate channel 2. dmaengine_slave_config() then you cnan do preare etc multiple times based on need. Yeah, I think we should just configure the slave id once and this would make the configuration simply. But by debugging, I find the dmaengine_slave_config() may be called more one times after requested by on user. So I put the slave id configuration in the channel request phase. And the effect is the same for exclusive channel. Thanks! Best Regards, Jingchang
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
> > How about change the filter_fn to follow:
> > static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
> > {
> > struct fsl_edma_filter_param *fparam = fn_param;
> > struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > unsigned char val;
> >
> > if (fsl_chan->edmamux->mux_id != fparam->mux_id)
> > return false;
> >
> > val = EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(fparam-
> >slot_id);
> > fsl_edmamux_config_chan(fsl_chan, val);
> > return true;
> > }
> > In fact the slot_id isn't need elsewhere, and if the filter return true,
> > This channel should be to this request. So no need to save the slave id,
> Right?
> something like
>
> static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
> {
> struct fsl_edma_filter_param *fparam = fn_param;
> struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
>
> if (fsl_chan->edmamux->mux_id != fparam->mux_id)
> return false;
> return true;
> }
>
> in thedriver which calls this:
>
> before prep:
>
> config->slave_id = val;
>
> dma_set_slave_config(chan, slave);
>
Do you mean the DMA_SLAVE_CONFIG device_control? Yeah, the slave driver could
pass
the slave_id. But the DMA_SLAVE_CONFIG may be called more than once, and the
eDMA
driver just needs to set the slave id once for any given channel, after that
the
transfer is transparent to the device.
On the other hand, the DMAMUX's setting procedure requires first disable the
dmamux
before setting, then if it is set in DMA_SLAVE_CONFIG, the repeated setting may
be
complex and unnecessary. The channel is occupied exclusively by the peripheral.
So, according the HW feature, I think the eDMA needs only set the slave id
once,
and since the of_dma helper has pass the slave id in on xlate, we can get and
set
the slave id here. How do you think about this?
Thanks!
Best Regards,
Jingchang
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Monday, September 02, 2013 2:37 PM
> To: Lu Jingchang-B35083
> Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
>
> On Mon, Sep 02, 2013 at 07:10:53AM +, Lu Jingchang-B35083 wrote:
> > > -Original Message-
> > > From: Vinod Koul [mailto:vinod.k...@intel.com]
> > > Sent: Monday, September 02, 2013 12:51 PM
> > > To: Lu Jingchang-B35083
> > > Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
> > > ker...@lists.infradead.org; devicet...@vger.kernel.org
> > > Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver
> support
> > >
> > > On Thu, Aug 29, 2013 at 03:32:04AM +, Lu Jingchang-B35083 wrote:
> > >
> > > Please use a right MUA and wrap your lines at 80chars...
>
> > > > [Lu Jingchang]
> > > No need to put your name :)
> > [Lu Jingchang-b35083]
> > Aha, the Microsoft Outlook adds this automatically with default option.
> You can change that!!
Thanks.
> > >
> > > > > > +static bool fsl_edma_filter_fn(struct dma_chan *chan, void
> > > *fn_param)
> > > > > > +{
> > > > > > + struct fsl_edma_filter_param *fparam = fn_param;
> > > > > > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > > > > > +
> > > > > > + if (fsl_chan->edmamux->mux_id != fparam->mux_id)
> > > > > > + return false;
> > > > > > +
> > > > > > + fsl_chan->slot_id = fparam->slot_id;
> > > > > > + chan->private = fn_param;
> > > > > why do you need to use chan->private?
> > > > [Lu Jingchang]
> > > > The private used here is to store the slot_id information, which
> must
> > > be used
> > > > by the DMAMUX in alloc_chan_resources function. Thanks.
> > > Why dont you pass this in struct dma_slave_config memeber slave_id
> for
> > > this.
> > [Lu Jingchang-b35083]
> > I will drop this private and setup the slave_id directly in the filter
> function.
> why in filter? before calling prepare function you can set the slave
> config
How about change the filter_fn to follow:
static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
{
struct fsl_edma_filter_param *fparam = fn_param;
struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
unsigned char val;
if (fsl_chan->edmamux->mux_id != fparam->mux_id)
return false;
val = EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(fparam->slot_id);
fsl_edmamux_config_chan(fsl_chan, val);
return true;
}
In fact the slot_id isn't need elsewhere, and if the filter return true,
This channel should be to this request. So no need to save the slave id, Right?
Best Regareds,
Jingchang
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Monday, September 02, 2013 12:51 PM
> To: Lu Jingchang-B35083
> Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
>
> On Thu, Aug 29, 2013 at 03:32:04AM +, Lu Jingchang-B35083 wrote:
>
> Please use a right MUA and wrap your lines at 80chars...
>
> >
> > >
> > > > + return -EINVAL;
> > > > + }
> > > > + return 0;
> > > > +
> > > > + default:
> > > > + return -ENXIO;
> > > > + }
> > > > +}
> > > > +
> > > > +static enum dma_status fsl_edma_tx_status(struct dma_chan *chan,
> > > > + dma_cookie_t cookie, struct dma_tx_state *txstate)
> > > > +{
> > > > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > > > +
> > > > + if (fsl_chan->status == DMA_ERROR)
> > > > + return DMA_ERROR;
> > > > +
> > > > + return dma_cookie_status(chan, cookie, txstate);
> > > this will tell if the DMA is completed or not only.
> > > You also need to calculate residue for the pending dma
> > >
> > > Since you support cyclic this should be done properly...
> > >
> > > also you cna take more help from vchan support to make your life
> > > simpler...
> > [Lu Jingchang]
> No need to put your name :)
[Lu Jingchang-b35083]
Aha, the Microsoft Outlook adds this automatically with default option.
>
> > Ok, if it is needed, I will add residue calculation in the next version.
> Yes this is needed
>
> > > > +static bool fsl_edma_filter_fn(struct dma_chan *chan, void
> *fn_param)
> > > > +{
> > > > + struct fsl_edma_filter_param *fparam = fn_param;
> > > > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > > > +
> > > > + if (fsl_chan->edmamux->mux_id != fparam->mux_id)
> > > > + return false;
> > > > +
> > > > + fsl_chan->slot_id = fparam->slot_id;
> > > > + chan->private = fn_param;
> > > why do you need to use chan->private?
> > [Lu Jingchang]
> > The private used here is to store the slot_id information, which must
> be used
> > by the DMAMUX in alloc_chan_resources function. Thanks.
> Why dont you pass this in struct dma_slave_config memeber slave_id for
> this.
[Lu Jingchang-b35083]
I will drop this private and setup the slave_id directly in the filter function.
The slave id is only used in first request phase, if filter is true, the channel
Should be to this slave.
Thanks.
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Monday, September 02, 2013 12:51 PM
To: Lu Jingchang-B35083
Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; devicet...@vger.kernel.org
Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
On Thu, Aug 29, 2013 at 03:32:04AM +, Lu Jingchang-B35083 wrote:
Please use a right MUA and wrap your lines at 80chars...
+ return -EINVAL;
+ }
+ return 0;
+
+ default:
+ return -ENXIO;
+ }
+}
+
+static enum dma_status fsl_edma_tx_status(struct dma_chan *chan,
+ dma_cookie_t cookie, struct dma_tx_state *txstate)
+{
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+
+ if (fsl_chan-status == DMA_ERROR)
+ return DMA_ERROR;
+
+ return dma_cookie_status(chan, cookie, txstate);
this will tell if the DMA is completed or not only.
You also need to calculate residue for the pending dma
Since you support cyclic this should be done properly...
also you cna take more help from vchan support to make your life
simpler...
[Lu Jingchang]
No need to put your name :)
[Lu Jingchang-b35083]
Aha, the Microsoft Outlook adds this automatically with default option.
Ok, if it is needed, I will add residue calculation in the next version.
Yes this is needed
+static bool fsl_edma_filter_fn(struct dma_chan *chan, void
*fn_param)
+{
+ struct fsl_edma_filter_param *fparam = fn_param;
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+
+ if (fsl_chan-edmamux-mux_id != fparam-mux_id)
+ return false;
+
+ fsl_chan-slot_id = fparam-slot_id;
+ chan-private = fn_param;
why do you need to use chan-private?
[Lu Jingchang]
The private used here is to store the slot_id information, which must
be used
by the DMAMUX in alloc_chan_resources function. Thanks.
Why dont you pass this in struct dma_slave_config memeber slave_id for
this.
[Lu Jingchang-b35083]
I will drop this private and setup the slave_id directly in the filter function.
The slave id is only used in first request phase, if filter is true, the channel
Should be to this slave.
Thanks.
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Monday, September 02, 2013 2:37 PM
To: Lu Jingchang-B35083
Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; devicet...@vger.kernel.org
Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
On Mon, Sep 02, 2013 at 07:10:53AM +, Lu Jingchang-B35083 wrote:
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Monday, September 02, 2013 12:51 PM
To: Lu Jingchang-B35083
Cc: shawn@linaro.org; linux-kernel@vger.kernel.org; linux-arm-
ker...@lists.infradead.org; devicet...@vger.kernel.org
Subject: Re: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver
support
On Thu, Aug 29, 2013 at 03:32:04AM +, Lu Jingchang-B35083 wrote:
Please use a right MUA and wrap your lines at 80chars...
[Lu Jingchang]
No need to put your name :)
[Lu Jingchang-b35083]
Aha, the Microsoft Outlook adds this automatically with default option.
You can change that!!
Thanks.
+static bool fsl_edma_filter_fn(struct dma_chan *chan, void
*fn_param)
+{
+ struct fsl_edma_filter_param *fparam = fn_param;
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+
+ if (fsl_chan-edmamux-mux_id != fparam-mux_id)
+ return false;
+
+ fsl_chan-slot_id = fparam-slot_id;
+ chan-private = fn_param;
why do you need to use chan-private?
[Lu Jingchang]
The private used here is to store the slot_id information, which
must
be used
by the DMAMUX in alloc_chan_resources function. Thanks.
Why dont you pass this in struct dma_slave_config memeber slave_id
for
this.
[Lu Jingchang-b35083]
I will drop this private and setup the slave_id directly in the filter
function.
why in filter? before calling prepare function you can set the slave
config
How about change the filter_fn to follow:
static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
{
struct fsl_edma_filter_param *fparam = fn_param;
struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
unsigned char val;
if (fsl_chan-edmamux-mux_id != fparam-mux_id)
return false;
val = EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(fparam-slot_id);
fsl_edmamux_config_chan(fsl_chan, val);
return true;
}
In fact the slot_id isn't need elsewhere, and if the filter return true,
This channel should be to this request. So no need to save the slave id, Right?
Best Regareds,
Jingchang
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
How about change the filter_fn to follow:
static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
{
struct fsl_edma_filter_param *fparam = fn_param;
struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
unsigned char val;
if (fsl_chan-edmamux-mux_id != fparam-mux_id)
return false;
val = EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(fparam-
slot_id);
fsl_edmamux_config_chan(fsl_chan, val);
return true;
}
In fact the slot_id isn't need elsewhere, and if the filter return true,
This channel should be to this request. So no need to save the slave id,
Right?
something like
static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
{
struct fsl_edma_filter_param *fparam = fn_param;
struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
if (fsl_chan-edmamux-mux_id != fparam-mux_id)
return false;
return true;
}
in thedriver which calls this:
before prep:
config-slave_id = val;
dma_set_slave_config(chan, slave);
Do you mean the DMA_SLAVE_CONFIG device_control? Yeah, the slave driver could
pass
the slave_id. But the DMA_SLAVE_CONFIG may be called more than once, and the
eDMA
driver just needs to set the slave id once for any given channel, after that
the
transfer is transparent to the device.
On the other hand, the DMAMUX's setting procedure requires first disable the
dmamux
before setting, then if it is set in DMA_SLAVE_CONFIG, the repeated setting may
be
complex and unnecessary. The channel is occupied exclusively by the peripheral.
So, according the HW feature, I think the eDMA needs only set the slave id
once,
and since the of_dma helper has pass the slave id in on xlate, we can get and
set
the slave id here. How do you think about this?
Thanks!
Best Regards,
Jingchang
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
> > + } else {
> since you support cyclic, is there a reasonw why you dont support
> pause/resume?
> is it hw issue or planned in future?
[Lu Jingchang]
The HW supports start/stop request from the peripheral dma request, I had
planned to add this in future as requested.
I will add this in the next version patch.
Thanks.
>
> > + return -EINVAL;
> > + }
> > + return 0;
> > +
> > + default:
> > + return -ENXIO;
> > + }
> > +}
> > +
> > +static enum dma_status fsl_edma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie, struct dma_tx_state *txstate)
> > +{
> > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > +
> > + if (fsl_chan->status == DMA_ERROR)
> > + return DMA_ERROR;
> > +
> > + return dma_cookie_status(chan, cookie, txstate);
> this will tell if the DMA is completed or not only.
> You also need to calculate residue for the pending dma
>
> Since you support cyclic this should be done properly...
>
> also you cna take more help from vchan support to make your life
> simpler...
[Lu Jingchang]
Ok, if it is needed, I will add residue calculation in the next version.
Thanks.
>
[...]
> > +static struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
> > + struct dma_chan *chan, dma_addr_t dma_addr, size_t buf_len,
> > + size_t period_len, enum dma_transfer_direction direction,
> > + unsigned long flags, void *context)
> > +{
> > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > + struct fsl_edma_desc *fsl_desc;
> > + dma_addr_t dma_buf_next;
> > + int sg_len, i;
> > + u32 src_addr, dst_addr, last_sg, nbytes;
> > + u16 soff, doff, iter;
> > +
> > + if (!fsl_chan->fsc.dir == DMA_MEM_TO_DEV &&
> > + !fsl_chan->fsc.dir == DMA_DEV_TO_MEM)
> > + return NULL;
> you are ignoring the direction arg. Also use is_slave_direction() helper
[...]
> > + if (!fsl_chan->fsc.dir == DMA_MEM_TO_DEV &&
> > + !fsl_chan->fsc.dir == DMA_DEV_TO_MEM)
> > + return NULL;
> ditto
[Lu Jingchang]
Thanks. I will call the helper function instead.
[...]
> > +static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
> > +{
> > + struct fsl_edma_filter_param *fparam = fn_param;
> > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > +
> > + if (fsl_chan->edmamux->mux_id != fparam->mux_id)
> > + return false;
> > +
> > + fsl_chan->slot_id = fparam->slot_id;
> > + chan->private = fn_param;
> why do you need to use chan->private?
[Lu Jingchang]
The private used here is to store the slot_id information, which must be used
by the DMAMUX in alloc_chan_resources function. Thanks.
> > + return true;
> > +}
> > +
> > +static struct dma_chan *fsl_edma_xlate(struct of_phandle_args
> *dma_spec,
> > + struct of_dma *ofdma)
> > +{
> > + dma_cap_mask_t mask;
> > + struct fsl_edma_filter_param fparam;
> > +
> > + if (dma_spec->args_count != 2)
> > + return NULL;
> > +
> > + fparam.mux_id = dma_spec->args[0];
> > + fparam.slot_id = dma_spec->args[1];
> > +
> > + dma_cap_zero(mask);
> > + dma_cap_set(DMA_SLAVE, mask);
> > + dma_cap_set(DMA_CYCLIC, mask);
> > + return dma_request_channel(mask, fsl_edma_filter_fn, (void
> *));
> > +}
> > +
> > +static int fsl_edma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > + struct fsl_edma_filter_param *data = chan->private;
> > + unsigned char val;
> > +
> > + val = EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(data->slot_id);
> okay, so what does the slot_id mean?
>
[Lu Jingchang]
slot_id is the request source id, a peripheral device ID. Each peripheral using
DMA has a ID that can be identified by the DMA engine. the peripheral ID must
be written to the DMAMUX configuration register to route the peripheral to DMA
engine channel. Thanks.
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod,
Could you please help review this patch? Could it be merged into your next
tree?
Thanks!
Best Regards,
Jingchang
> -Original Message-
> From: Lu Jingchang-B35083
> Sent: Friday, August 16, 2013 2:08 PM
> To: vinod.k...@intel.com
> Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Lu Jingchang-B35083; Wang Huan-
> B18965
> Subject: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
>
> Add Freescale enhanced direct memory(eDMA) controller support.
> The eDMA controller deploys DMAMUXs routing DMA request sources(slot) to
> eDMA channels.
> This module can be found on Vybrid and LS-1 SoCs.
>
> Signed-off-by: Alison Wang
> Signed-off-by: Jingchang Lu
> ---
> changes in v4:
> using exact compatible string in binding document.
>
> changes in v3:
> handle all pending interrupt one time.
> add protect lock on dma transfer complete handling.
> change desc and tcd alloc flag to GFP_NOWAIT.
> add sanity check and error messages.
>
> changes in v2:
> using generic dma-channels property instead of fsl,dma-channel.
> rename the binding document to fsl-edma.txt.
>
> Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++
> drivers/dma/Kconfig| 10 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-edma.c | 855
> +
> 4 files changed, 950 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
> create mode 100644 drivers/dma/fsl-edma.c
>
> diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt
> b/Documentation/devicetree/bindings/dma/fsl-edma.txt
> new file mode 100644
> index 000..95159da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
> @@ -0,0 +1,84 @@
> +* Freescale enhanced direct memory access(eDMA) Controller
> +
> +The eDMA engine deploys DMAMUXs routing request sources(slot) to eDMA
> +controller channels.
> +
> +* eDMA Controller
> +Required properties:
> +- compatible :
> + - "fsl,vf610-edma" for eDMA used similar to that on Vybrid vf610 SoC
> +- reg : Should contain eDMA registers location and length
> +- interrupts : Should contain eDMA interrupt
> +- interrupt-names : Should be "edma-tx" for tx interrupt and
> + "edma-err" for err interrupt
> +- #dma-cells : Must be <2>.
> + The first cell specifies the DMAMUX ID. Specific request source
> + can only be routed by specific DMAMUXs.
> + the second cell specifies the request source(slot) ID.
> + See include/dt-bindings/dma/-edma.h for all the supported
> + request source IDs.
> +- dma-channels : Number of channels supported by the controller
> +- fsl,dma-mux : Phandle of the DMAMUXs deployed by the controller
> +
> +
> +* DMAMUX
> +Required properties:
> +- reg : Should contain DMAMUX registers location and length
> +- fsl,dmamux-id : DMAMUX ID. DMAMUX IDs are unique in each eDMA
> controller.
> + inside one eDMA controller, specific request source can only be
> +routed by
> + one of its DMAMUXs.
> + However Specific request source may be routed to different eDMA
> +controller,
> + thus all the DMAMUXs sharing a the same request sources have the same
> ID.
> +- clocks : Phandle of the clock used by the DMAMUX
> +- clock-names : The clock names
> +
> +Below is the eDMA controller and DMAMUX association, and DMAMUX IDs
> +assignment On Vybrid vf610 SoC, DMAMUX0 and DMAMU3 share the same
> +request source group, and DMAMUX1 and DMAMU2 share the same request
> source group.
> +
> +eDMA controller DMAMUXs DMAMUX ID
> +-
> +eDMA0DMAMUX0 0
> + DMAMUX1 1
> +
> +eDMA1DMAMUX2 1
> + DMAMUX3 0
> +
> +Examples:
> +
> +edma0: edma@40018000 {
> + #dma-cells = <2>;
> + compatible = "fsl,vf610-edma";
> + reg = <0x40018000 0x2000>;
> + interrupts = <0 8 0x04>, <0 9 0x04>;
> + interrupt-names = "edma-tx", "edma-err";
> + dma-channels = <32>;
> + fsl,edma-mux = <>, <>; };
> +
> +dmamux0: dmamux@40024000 {
> + reg = <0x40024000 0x1000>;
> + fsl,dmamux-id = <0>;
> + clocks = < VF610_CLK_DMAMUX0>;
> + clock-names = "dmamux";
> +};
> +
> +
> +* DMA clients
> +DMA client drivers that uses the DMA function must use the format
> +described in the dma.txt f
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod,
Could you please help review this patch? Could it be merged into your next
tree?
Thanks!
Best Regards,
Jingchang
-Original Message-
From: Lu Jingchang-B35083
Sent: Friday, August 16, 2013 2:08 PM
To: vinod.k...@intel.com
Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; Lu Jingchang-B35083; Wang Huan-
B18965
Subject: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
Add Freescale enhanced direct memory(eDMA) controller support.
The eDMA controller deploys DMAMUXs routing DMA request sources(slot) to
eDMA channels.
This module can be found on Vybrid and LS-1 SoCs.
Signed-off-by: Alison Wang b18...@freescale.com
Signed-off-by: Jingchang Lu b35...@freescale.com
---
changes in v4:
using exact compatible string in binding document.
changes in v3:
handle all pending interrupt one time.
add protect lock on dma transfer complete handling.
change desc and tcd alloc flag to GFP_NOWAIT.
add sanity check and error messages.
changes in v2:
using generic dma-channels property instead of fsl,dma-channel.
rename the binding document to fsl-edma.txt.
Documentation/devicetree/bindings/dma/fsl-edma.txt | 84 ++
drivers/dma/Kconfig| 10 +
drivers/dma/Makefile | 1 +
drivers/dma/fsl-edma.c | 855
+
4 files changed, 950 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
create mode 100644 drivers/dma/fsl-edma.c
diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt
b/Documentation/devicetree/bindings/dma/fsl-edma.txt
new file mode 100644
index 000..95159da
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
@@ -0,0 +1,84 @@
+* Freescale enhanced direct memory access(eDMA) Controller
+
+The eDMA engine deploys DMAMUXs routing request sources(slot) to eDMA
+controller channels.
+
+* eDMA Controller
+Required properties:
+- compatible :
+ - fsl,vf610-edma for eDMA used similar to that on Vybrid vf610 SoC
+- reg : Should contain eDMA registers location and length
+- interrupts : Should contain eDMA interrupt
+- interrupt-names : Should be edma-tx for tx interrupt and
+ edma-err for err interrupt
+- #dma-cells : Must be 2.
+ The first cell specifies the DMAMUX ID. Specific request source
+ can only be routed by specific DMAMUXs.
+ the second cell specifies the request source(slot) ID.
+ See include/dt-bindings/dma/soc-edma.h for all the supported
+ request source IDs.
+- dma-channels : Number of channels supported by the controller
+- fsl,dma-mux : Phandle of the DMAMUXs deployed by the controller
+
+
+* DMAMUX
+Required properties:
+- reg : Should contain DMAMUX registers location and length
+- fsl,dmamux-id : DMAMUX ID. DMAMUX IDs are unique in each eDMA
controller.
+ inside one eDMA controller, specific request source can only be
+routed by
+ one of its DMAMUXs.
+ However Specific request source may be routed to different eDMA
+controller,
+ thus all the DMAMUXs sharing a the same request sources have the same
ID.
+- clocks : Phandle of the clock used by the DMAMUX
+- clock-names : The clock names
+
+Below is the eDMA controller and DMAMUX association, and DMAMUX IDs
+assignment On Vybrid vf610 SoC, DMAMUX0 and DMAMU3 share the same
+request source group, and DMAMUX1 and DMAMU2 share the same request
source group.
+
+eDMA controller DMAMUXs DMAMUX ID
+-
+eDMA0DMAMUX0 0
+ DMAMUX1 1
+
+eDMA1DMAMUX2 1
+ DMAMUX3 0
+
+Examples:
+
+edma0: edma@40018000 {
+ #dma-cells = 2;
+ compatible = fsl,vf610-edma;
+ reg = 0x40018000 0x2000;
+ interrupts = 0 8 0x04, 0 9 0x04;
+ interrupt-names = edma-tx, edma-err;
+ dma-channels = 32;
+ fsl,edma-mux = dmamux0, dmamux1; };
+
+dmamux0: dmamux@40024000 {
+ reg = 0x40024000 0x1000;
+ fsl,dmamux-id = 0;
+ clocks = clks VF610_CLK_DMAMUX0;
+ clock-names = dmamux;
+};
+
+
+* DMA clients
+DMA client drivers that uses the DMA function must use the format
+described in the dma.txt file, using a three-cell specifier for each
+channel: a phandle plus two integer cells as described above.
+
+Examples:
+
+sai2: sai@40031000 {
+ compatible = fsl,vf610-sai;
+ reg = 0x40031000 0x1000;
+ interrupts = 0 86 0x04;
+ clocks = clks VF610_CLK_SAI2;
+ clock-names = sai;
+ dma-names = tx, rx;
+ dmas = edma0 0 VF610_EDMA_MUXID0_SAI2_TX,
+ edma0 0 VF610_EDMA_MUXID0_SAI2_RX;
+ status = disabled;
+};
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index
6825957..f9a6e06 100644
--- a/drivers/dma/Kconfig
+++ b
RE: [PATCH v4 3/3] dma: Add Freescale eDMA engine driver support
+ } else {
since you support cyclic, is there a reasonw why you dont support
pause/resume?
is it hw issue or planned in future?
[Lu Jingchang]
The HW supports start/stop request from the peripheral dma request, I had
planned to add this in future as requested.
I will add this in the next version patch.
Thanks.
+ return -EINVAL;
+ }
+ return 0;
+
+ default:
+ return -ENXIO;
+ }
+}
+
+static enum dma_status fsl_edma_tx_status(struct dma_chan *chan,
+ dma_cookie_t cookie, struct dma_tx_state *txstate)
+{
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+
+ if (fsl_chan-status == DMA_ERROR)
+ return DMA_ERROR;
+
+ return dma_cookie_status(chan, cookie, txstate);
this will tell if the DMA is completed or not only.
You also need to calculate residue for the pending dma
Since you support cyclic this should be done properly...
also you cna take more help from vchan support to make your life
simpler...
[Lu Jingchang]
Ok, if it is needed, I will add residue calculation in the next version.
Thanks.
[...]
+static struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
+ struct dma_chan *chan, dma_addr_t dma_addr, size_t buf_len,
+ size_t period_len, enum dma_transfer_direction direction,
+ unsigned long flags, void *context)
+{
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+ struct fsl_edma_desc *fsl_desc;
+ dma_addr_t dma_buf_next;
+ int sg_len, i;
+ u32 src_addr, dst_addr, last_sg, nbytes;
+ u16 soff, doff, iter;
+
+ if (!fsl_chan-fsc.dir == DMA_MEM_TO_DEV
+ !fsl_chan-fsc.dir == DMA_DEV_TO_MEM)
+ return NULL;
you are ignoring the direction arg. Also use is_slave_direction() helper
[...]
+ if (!fsl_chan-fsc.dir == DMA_MEM_TO_DEV
+ !fsl_chan-fsc.dir == DMA_DEV_TO_MEM)
+ return NULL;
ditto
[Lu Jingchang]
Thanks. I will call the helper function instead.
[...]
+static bool fsl_edma_filter_fn(struct dma_chan *chan, void *fn_param)
+{
+ struct fsl_edma_filter_param *fparam = fn_param;
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+
+ if (fsl_chan-edmamux-mux_id != fparam-mux_id)
+ return false;
+
+ fsl_chan-slot_id = fparam-slot_id;
+ chan-private = fn_param;
why do you need to use chan-private?
[Lu Jingchang]
The private used here is to store the slot_id information, which must be used
by the DMAMUX in alloc_chan_resources function. Thanks.
+ return true;
+}
+
+static struct dma_chan *fsl_edma_xlate(struct of_phandle_args
*dma_spec,
+ struct of_dma *ofdma)
+{
+ dma_cap_mask_t mask;
+ struct fsl_edma_filter_param fparam;
+
+ if (dma_spec-args_count != 2)
+ return NULL;
+
+ fparam.mux_id = dma_spec-args[0];
+ fparam.slot_id = dma_spec-args[1];
+
+ dma_cap_zero(mask);
+ dma_cap_set(DMA_SLAVE, mask);
+ dma_cap_set(DMA_CYCLIC, mask);
+ return dma_request_channel(mask, fsl_edma_filter_fn, (void
*)fparam);
+}
+
+static int fsl_edma_alloc_chan_resources(struct dma_chan *chan)
+{
+ struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
+ struct fsl_edma_filter_param *data = chan-private;
+ unsigned char val;
+
+ val = EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(data-slot_id);
okay, so what does the slot_id mean?
[Lu Jingchang]
slot_id is the request source id, a peripheral device ID. Each peripheral using
DMA has a ID that can be identified by the DMA engine. the peripheral ID must
be written to the DMAMUX configuration register to route the peripheral to DMA
engine channel. Thanks.
Best Regards,
Jingchang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v3 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod,
Could you please help to review the Freescale eDMA driver support patch?
Many thanks.
Best Regards,
Jingchang
> -Original Message-
> From: Lu Jingchang-B35083
> Sent: Tuesday, August 06, 2013 2:05 PM
> To: vinod.k...@intel.com
> Cc: d...@fb.com; shawn@linaro.org; linux-ke...@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Lu Jingchang-B35083; Wang Huan-
> B18965; Li Xiaochun-B41219
> Subject: [PATCH v3 3/3] dma: Add Freescale eDMA engine driver support
>
> Add Freescale enhanced direct memory(eDMA) controller support.
> The eDMA controller deploys DMAMUXs routing DMA request sources(slot)
> to eDMA channels.
> This module can be found on Vybrid and LS-1 SoCs.
>
> Signed-off-by: Alison Wang
> Signed-off-by: Xiaochun Li
> Signed-off-by: Jingchang Lu
> ---
> changes in v3:
> handle all interrupt pending one time.
> add protect lock on dma transfer complete handling.
> change desc and tcd alloc flag to GFP_NOWAIT.
> add some sanity check and error messages.
>
> changes in v2:
> using generic dma-channels property instead of fsl,dma-channel.
> rename the binding document to fsl-edma.txt.
>
> Documentation/devicetree/bindings/dma/fsl-edma.txt | 83 ++
> drivers/dma/Kconfig| 10 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-edma.c | 855
> +
> 4 files changed, 949 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
> create mode 100644 drivers/dma/fsl-edma.c
>
> diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt
> b/Documentation/devicetree/bindings/dma/fsl-edma.txt
> new file mode 100644
> index 000..a75e87c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
> @@ -0,0 +1,83 @@
> +* Freescale enhanced direct memory access(eDMA) Controller
> +
> +The eDMA engine deploys DMAMUXs routing request sources(slot) to
> +eDMA controller channels.
> +
> +* eDMA Controller
> +Required properties:
> +- compatible : Should be "fsl,-edma"
> +- reg : Should contain eDMA registers location and length
> +- interrupts : Should contain eDMA interrupt
> +- interrupt-names : Should be "edma-tx" for tx interrupt and
> + "edma-err" for err interrupt
> +- #dma-cells : Must be <2>.
> + The first cell specifies the DMAMUX ID. Specific request source
> + can only be routed by specific DMAMUXs.
> + the second cell specifies the request source(slot) ID.
> + See include/dt-bindings/dma/-edma.h for all the supported
> + request source IDs.
> +- dma-channels : Number of channels supported by the controller
> +- fsl,dma-mux : Phandle of the DMAMUXs deployed by the controller
> +
> +
> +* DMAMUX
> +Required properties:
> +- reg : Should contain DMAMUX registers location and length
> +- fsl,dmamux-id : DMAMUX ID. DMAMUX IDs are unique in each eDMA
> controller.
> + inside one eDMA controller, specific request source can only be routed
> by
> + one of its DMAMUXs.
> + However Specific request source may be routed to different eDMA
> controller,
> + thus all the DMAMUXs sharing a the same request sources have the same
> ID.
> +- clocks : Phandle of the clock used by the DMAMUX
> +- clock-names : The clock names
> +
> +Below is the eDMA controller and DMAMUX association, and DMAMUX IDs
> assignment
> +On Vybrid vf610 SoC, DMAMUX0 and DMAMU3 share the same request source
> group,
> +and DMAMUX1 and DMAMU2 share the same request source group.
> +
> +eDMA controller DMAMUXs DMAMUX ID
> +-
> +eDMA0DMAMUX0 0
> + DMAMUX1 1
> +
> +eDMA1DMAMUX2 1
> + DMAMUX3 0
> +
> +Examples:
> +
> +edma0: edma@40018000 {
> + #dma-cells = <2>;
> + compatible = "fsl,vf610-edma";
> + reg = <0x40018000 0x2000>;
> + interrupts = <0 8 0x04>, <0 9 0x04>;
> + interrupt-names = "edma-tx", "edma-err";
> + dma-channels = <32>;
> + fsl,edma-mux = <>, <>;
> +};
> +
> +dmamux0: dmamux@40024000 {
> + reg = <0x40024000 0x1000>;
> + fsl,dmamux-id = <0>;
> + clocks = < VF610_CLK_DMAMUX0>;
> + clock-names = "dmamux";
> +};
> +
> +
> +* DMA clients
> +DMA client drivers that uses the DMA function must use the format
> described
> +in the dma.txt file, using a three-cell specifier for each channel: a
> phandle
&
RE: [PATCH v3 3/3] dma: Add Freescale eDMA engine driver support
Hi, Vinod,
Could you please help to review the Freescale eDMA driver support patch?
Many thanks.
Best Regards,
Jingchang
-Original Message-
From: Lu Jingchang-B35083
Sent: Tuesday, August 06, 2013 2:05 PM
To: vinod.k...@intel.com
Cc: d...@fb.com; shawn@linaro.org; linux-ke...@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; Lu Jingchang-B35083; Wang Huan-
B18965; Li Xiaochun-B41219
Subject: [PATCH v3 3/3] dma: Add Freescale eDMA engine driver support
Add Freescale enhanced direct memory(eDMA) controller support.
The eDMA controller deploys DMAMUXs routing DMA request sources(slot)
to eDMA channels.
This module can be found on Vybrid and LS-1 SoCs.
Signed-off-by: Alison Wang b18...@freescale.com
Signed-off-by: Xiaochun Li b41...@freescale.com
Signed-off-by: Jingchang Lu b35...@freescale.com
---
changes in v3:
handle all interrupt pending one time.
add protect lock on dma transfer complete handling.
change desc and tcd alloc flag to GFP_NOWAIT.
add some sanity check and error messages.
changes in v2:
using generic dma-channels property instead of fsl,dma-channel.
rename the binding document to fsl-edma.txt.
Documentation/devicetree/bindings/dma/fsl-edma.txt | 83 ++
drivers/dma/Kconfig| 10 +
drivers/dma/Makefile | 1 +
drivers/dma/fsl-edma.c | 855
+
4 files changed, 949 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
create mode 100644 drivers/dma/fsl-edma.c
diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt
b/Documentation/devicetree/bindings/dma/fsl-edma.txt
new file mode 100644
index 000..a75e87c
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
@@ -0,0 +1,83 @@
+* Freescale enhanced direct memory access(eDMA) Controller
+
+The eDMA engine deploys DMAMUXs routing request sources(slot) to
+eDMA controller channels.
+
+* eDMA Controller
+Required properties:
+- compatible : Should be fsl,chip-edma
+- reg : Should contain eDMA registers location and length
+- interrupts : Should contain eDMA interrupt
+- interrupt-names : Should be edma-tx for tx interrupt and
+ edma-err for err interrupt
+- #dma-cells : Must be 2.
+ The first cell specifies the DMAMUX ID. Specific request source
+ can only be routed by specific DMAMUXs.
+ the second cell specifies the request source(slot) ID.
+ See include/dt-bindings/dma/soc-edma.h for all the supported
+ request source IDs.
+- dma-channels : Number of channels supported by the controller
+- fsl,dma-mux : Phandle of the DMAMUXs deployed by the controller
+
+
+* DMAMUX
+Required properties:
+- reg : Should contain DMAMUX registers location and length
+- fsl,dmamux-id : DMAMUX ID. DMAMUX IDs are unique in each eDMA
controller.
+ inside one eDMA controller, specific request source can only be routed
by
+ one of its DMAMUXs.
+ However Specific request source may be routed to different eDMA
controller,
+ thus all the DMAMUXs sharing a the same request sources have the same
ID.
+- clocks : Phandle of the clock used by the DMAMUX
+- clock-names : The clock names
+
+Below is the eDMA controller and DMAMUX association, and DMAMUX IDs
assignment
+On Vybrid vf610 SoC, DMAMUX0 and DMAMU3 share the same request source
group,
+and DMAMUX1 and DMAMU2 share the same request source group.
+
+eDMA controller DMAMUXs DMAMUX ID
+-
+eDMA0DMAMUX0 0
+ DMAMUX1 1
+
+eDMA1DMAMUX2 1
+ DMAMUX3 0
+
+Examples:
+
+edma0: edma@40018000 {
+ #dma-cells = 2;
+ compatible = fsl,vf610-edma;
+ reg = 0x40018000 0x2000;
+ interrupts = 0 8 0x04, 0 9 0x04;
+ interrupt-names = edma-tx, edma-err;
+ dma-channels = 32;
+ fsl,edma-mux = dmamux0, dmamux1;
+};
+
+dmamux0: dmamux@40024000 {
+ reg = 0x40024000 0x1000;
+ fsl,dmamux-id = 0;
+ clocks = clks VF610_CLK_DMAMUX0;
+ clock-names = dmamux;
+};
+
+
+* DMA clients
+DMA client drivers that uses the DMA function must use the format
described
+in the dma.txt file, using a three-cell specifier for each channel: a
phandle
+plus two integer cells as described above.
+
+Examples:
+
+sai2: sai@40031000 {
+ compatible = fsl,vf610-sai;
+ reg = 0x40031000 0x1000;
+ interrupts = 0 86 0x04;
+ clocks = clks VF610_CLK_SAI2;
+ clock-names = sai;
+ dma-names = tx, rx;
+ dmas = edma0 0 VF610_EDMA_MUXID0_SAI2_TX,
+ edma0 0 VF610_EDMA_MUXID0_SAI2_RX;
+ status = disabled;
+};
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 6825957..f9a6e06 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -300,6 +300,16
RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Tuesday, August 06, 2013 12:25 PM
> To: Lu Jingchang-B35083
> Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Wang Huan-B18965; Li Xiaochun-
> B41219
> Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
>
> On Tue, Aug 06, 2013 at 01:24:31AM +, Lu Jingchang-B35083 wrote:
> > > -Original Message-
> > > From: Vinod Koul [mailto:vinod.k...@intel.com]
> > > Sent: Tuesday, August 06, 2013 12:35 AM
> > > To: Lu Jingchang-B35083
> > > Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
> > > linux-arm-ker...@lists.infradead.org; Wang Huan-B18965; Li Xiaochun-
> > > B41219
> > > Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver
> > > support
> > > > +
> > > > +static void fsl_edma_free_desc(struct virt_dma_desc *vdesc) {
> > > > + struct fsl_edma_desc *fsl_desc;
> > > > + int i;
> > > > +
> > > > + fsl_desc = to_fsl_edma_desc(vdesc);
> > > > + 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);
> > > > + kfree(fsl_desc);
> > > should this be called with lock held or not?
> > [Lu Jingchang-B35083]
> > The desc list to be freed is got with lock held, and the free for each
> desc is independent, and the lock is not needed. Thanks.
> Would be apt to add this comment in the code, so that people know this
> function needs to be always called with lock held!
[Lu Jingchang-B35083]
Sorry, this function is called without lock held, I mean that the free()
doesn't need the lock held,
just as other drivers do.
It is called from vchan_free_chan_resources().
Thanks!
Best Regards,
Jingchang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Tuesday, August 06, 2013 12:35 AM
> To: Lu Jingchang-B35083
> Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; Wang Huan-B18965; Li Xiaochun-
> B41219
> Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
> > +
> > +static void fsl_edma_free_desc(struct virt_dma_desc *vdesc) {
> > + struct fsl_edma_desc *fsl_desc;
> > + int i;
> > +
> > + fsl_desc = to_fsl_edma_desc(vdesc);
> > + 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);
> > + kfree(fsl_desc);
> should this be called with lock held or not?
[Lu Jingchang-B35083]
The desc list to be freed is got with lock held, and the free for each desc is
independent, and the lock is not needed. Thanks.
Best Regards,
Jingchang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Lothar Waßmann [mailto:l...@karo-electronics.de]
> Sent: Monday, August 05, 2013 3:54 PM
> To: Lu Jingchang-B35083
> Cc: vinod.k...@intel.com; Li Xiaochun-B41219; Wang Huan-B18965; linux-
> ker...@vger.kernel.org; d...@fb.com; shawn@linaro.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
> [...]
> > + fsl_desc->n_tcds = sg_len;
> > + for (i = 0; i < sg_len; i++) {
> > + fsl_desc->tcd[i].vtcd = dma_pool_alloc(fsl_chan->tcd_pool,
> > + GFP_ATOMIC, _desc->tcd[i].ptcd);
> >
> Why is this called with GFP_ATOMIC while fsl_desc above is being
> allocated with GFP_KERNEL?
[Lu Jingchang-B35083]
I will make these consistently. Thanks.
>
> > + for (ch = 0; ch < 32; ch++)
> > + if (intr & (0x1 << ch))
> > + break;
> > +
> What, if IRQs for multiple channels are pending at the same time?
> You could handle them all in one go without extra calls of the IRQ
> handler.
[Lu Jingchang-B35083]
Yes, It will be more efficiently to handle all the irqs once. Thanks.
>
> > + writeb(EDMA_CINT_CINT(ch), base_addr + EDMA_CINT);
> > +
> > + fsl_chan = _edma->chans[ch];
> > +
> > + if (!fsl_chan->edesc->iscyclic) {
> > + list_del(_chan->edesc->vdesc.node);
> >
> Ain't there any protection needed for the list operation?
[Lu Jingchang-B35083]
Protection is needed indeed, I will fix this, thanks.
>
> > + clk_prepare_enable(fsl_edmamux->clk);
> >
> What, if this fails?
[Lu Jingchang-B35083]
I will add code to check the return value. Thanks.
>
Best Regards,
Jingchang
RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Lothar Waßmann [mailto:l...@karo-electronics.de]
Sent: Monday, August 05, 2013 3:54 PM
To: Lu Jingchang-B35083
Cc: vinod.k...@intel.com; Li Xiaochun-B41219; Wang Huan-B18965; linux-
ker...@vger.kernel.org; d...@fb.com; shawn@linaro.org; linux-arm-
ker...@lists.infradead.org
Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
[...]
+ fsl_desc-n_tcds = sg_len;
+ for (i = 0; i sg_len; i++) {
+ fsl_desc-tcd[i].vtcd = dma_pool_alloc(fsl_chan-tcd_pool,
+ GFP_ATOMIC, fsl_desc-tcd[i].ptcd);
Why is this called with GFP_ATOMIC while fsl_desc above is being
allocated with GFP_KERNEL?
[Lu Jingchang-B35083]
I will make these consistently. Thanks.
+ for (ch = 0; ch 32; ch++)
+ if (intr (0x1 ch))
+ break;
+
What, if IRQs for multiple channels are pending at the same time?
You could handle them all in one go without extra calls of the IRQ
handler.
[Lu Jingchang-B35083]
Yes, It will be more efficiently to handle all the irqs once. Thanks.
+ writeb(EDMA_CINT_CINT(ch), base_addr + EDMA_CINT);
+
+ fsl_chan = fsl_edma-chans[ch];
+
+ if (!fsl_chan-edesc-iscyclic) {
+ list_del(fsl_chan-edesc-vdesc.node);
Ain't there any protection needed for the list operation?
[Lu Jingchang-B35083]
Protection is needed indeed, I will fix this, thanks.
+ clk_prepare_enable(fsl_edmamux-clk);
What, if this fails?
[Lu Jingchang-B35083]
I will add code to check the return value. Thanks.
Best Regards,
Jingchang
RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Tuesday, August 06, 2013 12:35 AM
To: Lu Jingchang-B35083
Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; Wang Huan-B18965; Li Xiaochun-
B41219
Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
+
+static void fsl_edma_free_desc(struct virt_dma_desc *vdesc) {
+ struct fsl_edma_desc *fsl_desc;
+ int i;
+
+ fsl_desc = to_fsl_edma_desc(vdesc);
+ 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);
+ kfree(fsl_desc);
should this be called with lock held or not?
[Lu Jingchang-B35083]
The desc list to be freed is got with lock held, and the free for each desc is
independent, and the lock is not needed. Thanks.
Best Regards,
Jingchang
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Tuesday, August 06, 2013 12:25 PM
To: Lu Jingchang-B35083
Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; Wang Huan-B18965; Li Xiaochun-
B41219
Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver support
On Tue, Aug 06, 2013 at 01:24:31AM +, Lu Jingchang-B35083 wrote:
-Original Message-
From: Vinod Koul [mailto:vinod.k...@intel.com]
Sent: Tuesday, August 06, 2013 12:35 AM
To: Lu Jingchang-B35083
Cc: d...@fb.com; shawn@linaro.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; Wang Huan-B18965; Li Xiaochun-
B41219
Subject: Re: [PATCH v2 3/3] dma: Add Freescale eDMA engine driver
support
+
+static void fsl_edma_free_desc(struct virt_dma_desc *vdesc) {
+ struct fsl_edma_desc *fsl_desc;
+ int i;
+
+ fsl_desc = to_fsl_edma_desc(vdesc);
+ 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);
+ kfree(fsl_desc);
should this be called with lock held or not?
[Lu Jingchang-B35083]
The desc list to be freed is got with lock held, and the free for each
desc is independent, and the lock is not needed. Thanks.
Would be apt to add this comment in the code, so that people know this
function needs to be always called with lock held!
[Lu Jingchang-B35083]
Sorry, this function is called without lock held, I mean that the free()
doesn't need the lock held,
just as other drivers do.
It is called from vchan_free_chan_resources().
Thanks!
Best Regards,
Jingchang
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4] clocksource: add Vybrid Family pit timer support
>-Original Message-
>From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org]
>Sent: Tuesday, May 21, 2013 6:14 PM
>To: Lu Jingchang-B35083
>Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
>john.stu...@linaro.org; t...@linutronix.de; shawn@linaro.org;
>s.ha...@pengutronix.de
>Subject: Re: [PATCH v4] clocksource: add Vybrid Family pit timer support
>
>On 05/21/2013 09:27 AM, Jingchang Lu wrote:
>> Add Freescale Vybrid Family period interrupt timer support.
>>
>> Signed-off-by: Jingchang Lu
>> ---
>> v4:
>> use family name names driver and symbol instead of SoC name.
>> remove redundant code.
>> use BUG_ON instead of WARN_ON.
>> add necessory comment information.
>>
>> drivers/clocksource/Kconfig | 5 +
>> drivers/clocksource/Makefile| 1 +
>> drivers/clocksource/mvf_pit_timer.c | 187
>>
>> 3 files changed, 193 insertions(+)
>> create mode 100644 drivers/clocksource/mvf_pit_timer.c
>
>[ ... ]
>
>> +
>> +static int pit_set_next_event(unsigned long delta,
>> +struct clock_event_device *unused) {
>> +/*
>> + * set a new value to PITLDVAL register will not restart the timer,
>> + * to abort the current cycle and start a timer period with the new
>> + * value, the timer must be disabled and enabled again.
>> + * and the PITLAVAL should be set to delta minus one.
>
>Why delta "minus one" ?
[Lu Jingchang-B35083]
This is required by the PIT hardware, it is a down counter, the PITLAVAL
register should be set to (delta-1), it will raise an interrupt when it counts
down to 0. Thanks!
>
>> + */
>> +pit_timer_disable();
>> +__raw_writel(delta - 1, clkevt_base + PITLDVAL);
>> +pit_timer_enable();
>> +
>> +return 0;
>> +}
>> +
>> +static void pit_set_mode(enum clock_event_mode mode,
>> +struct clock_event_device *evt)
>> +{
>> +switch (mode) {
>> + case CLOCK_EVT_MODE_PERIODIC:
>> +pit_timer_disable();
>> +__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
>> +pit_timer_enable();
>
>You can call pit_set_next_event(cycle_per_jiffy, evt) instead of adding
>redundant code, no ?
[Lu Jingchang-B35083]
Yes, I will do that. Thanks!
>
>> +break;
>> +default:
>> +break;
>> +}
>> +}
>
>[ ... ]
>
>> +
>> +static struct clock_event_device clockevent_pit = {
>> +.name = "MVF pit timer",
>> +.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>> +.set_mode = pit_set_mode,
>> +.set_next_event = pit_set_next_event,
>> +.rating = 300,
>> +};
>> +
>> +static struct irqaction pit_timer_irq = {
>> +.name = "MVF pit timer",
>> +.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
>
>Did you take into account Thomas's comment ?
[Lu Jingchang-B35083]
Sorry, I misunderstand Thomas's meaning, I will remove IRQF_DISABLED flag.
Thanks!
>
>> +.handler = pit_timer_interrupt,
>> +.dev_id = _pit,
>> +};
>> +
>> +static int __init pit_clockevent_init(struct clk *pit_clk, int irq) {
>> +unsigned int c = clk_get_rate(pit_clk);
>> +
>> +__raw_writel(0, clkevt_base + PITTCTRL);
>> +__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
>> +
>> +clockevent_pit.cpumask = cpumask_of(0);
>
>The irq initialization is missing.
>
> clockevent_pit.irq = irq;
>
[Lu Jingchang-B35083]
Yes, I will add this. Thanks!
>> +clockevents_config_and_register(_pit, c, 2, 0x);
>
>Is it possible to have a comment with the justification for these values ?
[Lu Jingchang-B35083]
Yes, I will add a comment for these values. Thanks!
>
>> +
>> +BUG_ON(setup_irq(irq, _timer_irq));
>> +return 0;
>
>Everything is ok if you can't setup your timer ?
>
>> +}
>> +
>> +static void __init pit_timer_init(struct device_node *np) {
>> +struct clk *pit_clk;
>> +void __iomem *timer_base;
>> +int irq;
>> +
>> +timer_base = of_iomap(np, 0);
>> +BUG_ON(!timer_base);
>
>You raise a bug and then you go ahead setting up the address with an
>invalid value, leading to a random crash.
[Lu Jingchang-B35083]
The BUG_ON() will call BUG() if condition is true. It will print the stack
trace and the current process will die, it won't g
RE: [PATCH v4] clocksource: add Vybrid Family pit timer support
-Original Message-
From: Daniel Lezcano [mailto:daniel.lezc...@linaro.org]
Sent: Tuesday, May 21, 2013 6:14 PM
To: Lu Jingchang-B35083
Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
john.stu...@linaro.org; t...@linutronix.de; shawn@linaro.org;
s.ha...@pengutronix.de
Subject: Re: [PATCH v4] clocksource: add Vybrid Family pit timer support
On 05/21/2013 09:27 AM, Jingchang Lu wrote:
Add Freescale Vybrid Family period interrupt timer support.
Signed-off-by: Jingchang Lu b35...@freescale.com
---
v4:
use family name names driver and symbol instead of SoC name.
remove redundant code.
use BUG_ON instead of WARN_ON.
add necessory comment information.
drivers/clocksource/Kconfig | 5 +
drivers/clocksource/Makefile| 1 +
drivers/clocksource/mvf_pit_timer.c | 187
3 files changed, 193 insertions(+)
create mode 100644 drivers/clocksource/mvf_pit_timer.c
[ ... ]
+
+static int pit_set_next_event(unsigned long delta,
+struct clock_event_device *unused) {
+/*
+ * set a new value to PITLDVAL register will not restart the timer,
+ * to abort the current cycle and start a timer period with the new
+ * value, the timer must be disabled and enabled again.
+ * and the PITLAVAL should be set to delta minus one.
Why delta minus one ?
[Lu Jingchang-B35083]
This is required by the PIT hardware, it is a down counter, the PITLAVAL
register should be set to (delta-1), it will raise an interrupt when it counts
down to 0. Thanks!
+ */
+pit_timer_disable();
+__raw_writel(delta - 1, clkevt_base + PITLDVAL);
+pit_timer_enable();
+
+return 0;
+}
+
+static void pit_set_mode(enum clock_event_mode mode,
+struct clock_event_device *evt)
+{
+switch (mode) {
+case CLOCK_EVT_MODE_PERIODIC:
+pit_timer_disable();
+__raw_writel(cycle_per_jiffy - 1, clkevt_base + PITLDVAL);
+pit_timer_enable();
You can call pit_set_next_event(cycle_per_jiffy, evt) instead of adding
redundant code, no ?
[Lu Jingchang-B35083]
Yes, I will do that. Thanks!
+break;
+default:
+break;
+}
+}
[ ... ]
+
+static struct clock_event_device clockevent_pit = {
+.name = MVF pit timer,
+.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+.set_mode = pit_set_mode,
+.set_next_event = pit_set_next_event,
+.rating = 300,
+};
+
+static struct irqaction pit_timer_irq = {
+.name = MVF pit timer,
+.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
Did you take into account Thomas's comment ?
[Lu Jingchang-B35083]
Sorry, I misunderstand Thomas's meaning, I will remove IRQF_DISABLED flag.
Thanks!
+.handler= pit_timer_interrupt,
+.dev_id = clockevent_pit,
+};
+
+static int __init pit_clockevent_init(struct clk *pit_clk, int irq) {
+unsigned int c = clk_get_rate(pit_clk);
+
+__raw_writel(0, clkevt_base + PITTCTRL);
+__raw_writel(PITTFLG_TIF, clkevt_base + PITTFLG);
+
+clockevent_pit.cpumask = cpumask_of(0);
The irq initialization is missing.
clockevent_pit.irq = irq;
[Lu Jingchang-B35083]
Yes, I will add this. Thanks!
+clockevents_config_and_register(clockevent_pit, c, 2, 0x);
Is it possible to have a comment with the justification for these values ?
[Lu Jingchang-B35083]
Yes, I will add a comment for these values. Thanks!
+
+BUG_ON(setup_irq(irq, pit_timer_irq));
+return 0;
Everything is ok if you can't setup your timer ?
+}
+
+static void __init pit_timer_init(struct device_node *np) {
+struct clk *pit_clk;
+void __iomem *timer_base;
+int irq;
+
+timer_base = of_iomap(np, 0);
+BUG_ON(!timer_base);
You raise a bug and then you go ahead setting up the address with an
invalid value, leading to a random crash.
[Lu Jingchang-B35083]
The BUG_ON() will call BUG() if condition is true. It will print the stack
trace and the current process will die, it won't go ahead, won't it? Thanks!
+/*
+ * PIT0 and PIT1 can be chained to build a 64-bit timer,
+ * so choose PIT2 as clocksource, PIT3 as clockevent device,
+ * and leave PIT0 and PIT1 unused for anyone else who needs them.
+ */
+clksrc_base = timer_base + PITn_OFFSET(2);
+clkevt_base = timer_base + PITn_OFFSET(3);
+
+irq = irq_of_parse_and_map(np, 0);
+
+pit_clk = of_clk_get(np, 0);
+BUG_ON(IS_ERR(pit_clk));
+
+BUG_ON(clk_prepare_enable(pit_clk));
Same here.
+cycle_per_jiffy = clk_get_rate(pit_clk)/(HZ);
+
+/* enable the pit module */
+__raw_writel(~PITMCR_MDIS, timer_base + PITMCR);
+
+BUG_ON(pit_clocksource_init(pit_clk));
+
+pit_clockevent_init(pit_clk, irq);
Please, remove these BUG_ON
RE: [PATCH v3] clocksource: add MVF600 pit timer support
>-Original Message- >From: Shawn Guo [mailto:shawn@linaro.org] >Sent: Monday, May 20, 2013 11:21 AM >To: Lu Jingchang-B35083 >Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; >john.stu...@linaro.org; t...@linutronix.de; s.ha...@pengutronix.de >Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support > >On Mon, May 20, 2013 at 03:08:54AM +, Lu Jingchang-B35083 wrote: >> [Lu Jingchang-B35083] >> I am not sure MVF5xx and MVF7xx have the same PIT block, if you have >information that it is the same on other Vybrid SoCs, it is ok to change >the driver name to mvf. Thanks! > >Even the PIT block on other Vybrid SoCs have differences from the one >integrated on mvf600, we can handle them using device tree compatible >string. And that's why we need to encode SoC name in compatible. > [Lu Jingchang-B35083] Ok, I will rename the driver and the configure option, thanks! >> [Lu Jingchang-B35083] >> PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3 >are selected as the clocksource and clockevent device, and leave PIT0 and >PIT1 unused for anyone else who needs them. Thanks! > >This could be an useful info to be in the comment. > [Lu Jingchang-B35083] I will add a comment for this, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] clocksource: add MVF600 pit timer support
>-Original Message- >From: Shawn Guo [mailto:shawn@linaro.org] >Sent: Monday, May 20, 2013 10:28 AM >To: Lu Jingchang-B35083 >Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; >john.stu...@linaro.org; t...@linutronix.de; s.ha...@pengutronix.de >Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support > >On Thu, May 16, 2013 at 02:15:24PM +0800, Jingchang Lu wrote: >> Add Freescale Vybrid MVF600 period interrupt timer support. >> >> Signed-off-by: Jingchang Lu >> --- >> v3: >> move the pit driver to drivers/clocksource. >> >> drivers/clocksource/Kconfig| 5 + >> drivers/clocksource/Makefile | 1 + >> drivers/clocksource/mvf600_pit_timer.c | 224 >> + > >This is a driver for PIT, a timer IP block found on mvf family. Even >though we prefer to use particular SoC name to specify the compatibility >and version of the block, all the versions of the IP block should be >ideally supported by single PIT driver. This is just like we have drivers >spi-imx and i2c-imx support all SPI and I2C controllers found on IMX SoCs. >That said, I suggest you use family name "mvf" to name the driver, Kconfig >symbol etc. and use "mvf600" only in compatible string. > [Lu Jingchang-B35083] I am not sure MVF5xx and MVF7xx have the same PIT block, if you have information that it is the same on other Vybrid SoCs, it is ok to change the driver name to mvf. Thanks! >> 3 files changed, 230 insertions(+) >> + >> +/* choose PIT2 as clocksource, PIT3 as clockevent dev */ > >Reading the comment, I have a natural question - what PIT0 and PIT1 are >used for? > [Lu Jingchang-B35083] PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3 are selected as the clocksource and clockevent device, and leave PIT0 and PIT1 unused for anyone else who needs them. Thanks! > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] clocksource: add MVF600 pit timer support
-Original Message- From: Shawn Guo [mailto:shawn@linaro.org] Sent: Monday, May 20, 2013 10:28 AM To: Lu Jingchang-B35083 Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; john.stu...@linaro.org; t...@linutronix.de; s.ha...@pengutronix.de Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support On Thu, May 16, 2013 at 02:15:24PM +0800, Jingchang Lu wrote: Add Freescale Vybrid MVF600 period interrupt timer support. Signed-off-by: Jingchang Lu b35...@freescale.com --- v3: move the pit driver to drivers/clocksource. drivers/clocksource/Kconfig| 5 + drivers/clocksource/Makefile | 1 + drivers/clocksource/mvf600_pit_timer.c | 224 + This is a driver for PIT, a timer IP block found on mvf family. Even though we prefer to use particular SoC name to specify the compatibility and version of the block, all the versions of the IP block should be ideally supported by single PIT driver. This is just like we have drivers spi-imx and i2c-imx support all SPI and I2C controllers found on IMX SoCs. That said, I suggest you use family name mvf to name the driver, Kconfig symbol etc. and use mvf600 only in compatible string. [Lu Jingchang-B35083] I am not sure MVF5xx and MVF7xx have the same PIT block, if you have information that it is the same on other Vybrid SoCs, it is ok to change the driver name to mvf. Thanks! 3 files changed, 230 insertions(+) + +/* choose PIT2 as clocksource, PIT3 as clockevent dev */ Reading the comment, I have a natural question - what PIT0 and PIT1 are used for? [Lu Jingchang-B35083] PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3 are selected as the clocksource and clockevent device, and leave PIT0 and PIT1 unused for anyone else who needs them. Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] clocksource: add MVF600 pit timer support
-Original Message- From: Shawn Guo [mailto:shawn@linaro.org] Sent: Monday, May 20, 2013 11:21 AM To: Lu Jingchang-B35083 Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; john.stu...@linaro.org; t...@linutronix.de; s.ha...@pengutronix.de Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support On Mon, May 20, 2013 at 03:08:54AM +, Lu Jingchang-B35083 wrote: [Lu Jingchang-B35083] I am not sure MVF5xx and MVF7xx have the same PIT block, if you have information that it is the same on other Vybrid SoCs, it is ok to change the driver name to mvf. Thanks! Even the PIT block on other Vybrid SoCs have differences from the one integrated on mvf600, we can handle them using device tree compatible string. And that's why we need to encode SoC name in compatible. [Lu Jingchang-B35083] Ok, I will rename the driver and the configure option, thanks! [Lu Jingchang-B35083] PIT0 and PIT1 can be chained to build a 64-bit timer, so PIT2 and PIT3 are selected as the clocksource and clockevent device, and leave PIT0 and PIT1 unused for anyone else who needs them. Thanks! This could be an useful info to be in the comment. [Lu Jingchang-B35083] I will add a comment for this, thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] clocksource: add MVF600 pit timer support
>-Original Message-
>From: Thomas Gleixner [mailto:t...@linutronix.de]
>Sent: Thursday, May 16, 2013 10:05 PM
>To: Lu Jingchang-B35083
>Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
>john.stu...@linaro.org; shawn@linaro.org; s.ha...@pengutronix.de
>Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support
>
>> +static int pit_set_next_event(unsigned long delta,
>> +struct clock_event_device *unused) {
>> +pit_timer_disable();
>> +__raw_writel(delta - 1, clkevt_base + PITLDVAL);
>> +pit_irq_acknowledge();
>> +pit_timer_enable();
>
>It would be much more interesting to comment, why you need to acknowlegde
>the timer here.
[Lu Jingchang-B35083]
The pit is a period load and count timer, ack here is to make sure the int
flag is clear,
I will check if the ack here is needed and remove the redundant code. Thanks!
>
>> +static irqreturn_t pit_timer_interrupt(int irq, void *dev_id) {
>> +struct clock_event_device *evt = _pit;
>> +
>> +pit_irq_acknowledge();
>> +
>> +if (clockevent_mode == CLOCK_EVT_MODE_ONESHOT)
>> +pit_timer_disable();
>
>So in oneshot mode you do:
>
> pit_irq_ack()
> pit_timer_disable()
>
> set_next_event()
> pit_timer_disable()
> write_new_value()
> pit_irq_ack()
> pit_timer_enable()
>
>Not really the most efficient way in the interrupt fast path, right?
[Lu Jingchang-B35083]
The pit hardware doesn't support oneshot timer. So for oneshot mode event,
software need to disable and enable the timer each time.
The timers generate interrupt at periodic intervals, when enabled. The timers
load the start
values as specified in their LDVAL registers, count down to 0, generate an INT
and then reload the respective
start value again. Each time a timer reaches 0, it will set the interrupt flag.
I will remove the redundant irq_ack.
Thanks!
>
>> +evt->event_handler(evt);
>> +
>> +clockevents_config_and_register(_pit, c, 0x100,
>> +0xff00);
>
>0x100 and 0xff00 ?? Random numbers pulled out of what?
[Lu Jingchang-B35083]
It seems too small delta is not reasonable, and the max delta value is not
sensible.
So refer to some other platforms, I set it to 0x100-0xff00. I'm OK to set
it to 0x2~0x.
Thanks!
>
>> +
>> +timer_base = of_iomap(np, 0);
>> +WARN_ON(!timer_base);
>
>Great, timer_base is NULL and you just emit a warning and then proceed? So
>instead of either bailing out or crashing the machine right away you let
>it randomly die with the first access.
[Lu Jingchang-B35083]
I will use BUG_ON() instead of WARN_ON(). Thanks!
>
>> +
>> +clk_prepare_enable(pit_clk);
>
>And while you're worried about the core code sending you random crap, you
>assume that this call always succeeds.
[Lu Jingchang-B35083]
I will check the return value for that call. Thanks!
>> +__raw_writel(0, clksrc_base + PITTCTRL);
>> +__raw_writel(0x, clksrc_base + PITLDVAL);
>
>And of this? Why isn't the setup done in the relevant init functions?
[Lu Jingchang-B35083]
I will move these to relevant init functions. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] clocksource: add MVF600 pit timer support
-Original Message-
From: Thomas Gleixner [mailto:t...@linutronix.de]
Sent: Thursday, May 16, 2013 10:05 PM
To: Lu Jingchang-B35083
Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
john.stu...@linaro.org; shawn@linaro.org; s.ha...@pengutronix.de
Subject: Re: [PATCH v3] clocksource: add MVF600 pit timer support
+static int pit_set_next_event(unsigned long delta,
+struct clock_event_device *unused) {
+pit_timer_disable();
+__raw_writel(delta - 1, clkevt_base + PITLDVAL);
+pit_irq_acknowledge();
+pit_timer_enable();
It would be much more interesting to comment, why you need to acknowlegde
the timer here.
[Lu Jingchang-B35083]
The pit is a period load and count timer, ack here is to make sure the int
flag is clear,
I will check if the ack here is needed and remove the redundant code. Thanks!
+static irqreturn_t pit_timer_interrupt(int irq, void *dev_id) {
+struct clock_event_device *evt = clockevent_pit;
+
+pit_irq_acknowledge();
+
+if (clockevent_mode == CLOCK_EVT_MODE_ONESHOT)
+pit_timer_disable();
So in oneshot mode you do:
pit_irq_ack()
pit_timer_disable()
set_next_event()
pit_timer_disable()
write_new_value()
pit_irq_ack()
pit_timer_enable()
Not really the most efficient way in the interrupt fast path, right?
[Lu Jingchang-B35083]
The pit hardware doesn't support oneshot timer. So for oneshot mode event,
software need to disable and enable the timer each time.
The timers generate interrupt at periodic intervals, when enabled. The timers
load the start
values as specified in their LDVAL registers, count down to 0, generate an INT
and then reload the respective
start value again. Each time a timer reaches 0, it will set the interrupt flag.
I will remove the redundant irq_ack.
Thanks!
+evt-event_handler(evt);
+
+clockevents_config_and_register(clockevent_pit, c, 0x100,
+0xff00);
0x100 and 0xff00 ?? Random numbers pulled out of what?
[Lu Jingchang-B35083]
It seems too small delta is not reasonable, and the max delta value is not
sensible.
So refer to some other platforms, I set it to 0x100-0xff00. I'm OK to set
it to 0x2~0x.
Thanks!
+
+timer_base = of_iomap(np, 0);
+WARN_ON(!timer_base);
Great, timer_base is NULL and you just emit a warning and then proceed? So
instead of either bailing out or crashing the machine right away you let
it randomly die with the first access.
[Lu Jingchang-B35083]
I will use BUG_ON() instead of WARN_ON(). Thanks!
+
+clk_prepare_enable(pit_clk);
And while you're worried about the core code sending you random crap, you
assume that this call always succeeds.
[Lu Jingchang-B35083]
I will check the return value for that call. Thanks!
+__raw_writel(0, clksrc_base + PITTCTRL);
+__raw_writel(0x, clksrc_base + PITLDVAL);
And of this? Why isn't the setup done in the relevant init functions?
[Lu Jingchang-B35083]
I will move these to relevant init functions. Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
