* [4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Baolin Wang @ 2019-04-30 8:34 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 30-04-19, 13:30, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > > >
> > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > From: Eric Long <eric.long@unisoc.com>
> > > > > >
> > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > device validation in filter function to check if the correct controller
> > > > > > to be requested.
> > > > > >
> > > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > > ---
> > > > > > drivers/dma/sprd-dma.c | 5 +++++
> > > > > > 1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > index 0f92e60..9f99d4b 100644
> > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > {
> > > > > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > + struct of_phandle_args *dma_spec =
> > > > > > + container_of(param, struct of_phandle_args, args[0]);
> > > > > > u32 slave_id = *(u32 *)param;
> > > > > >
> > > > > > + if (chan->device->dev->of_node != dma_spec->np)
> > > > >
> > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > be useless!
> > > >
> > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > complicated than current solution. Since we need introduce one
> > > > structure to save the node to validate in the filter function like
> > > > below, which seems make things complicated. But if you still like to
> > > > use of_dma_find_controller(), I can change to use it in next version.
> > >
> > > Sorry I should have clarified more..
> > >
> > > of_dma_find_controller() is called by xlate, so you already run this
> > > check, so why use this :)
> >
> > The of_dma_find_controller() can save the requested device node into
> > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > dma_request_channel() to request one channel, but it did not validate
> > the device node to find the corresponding dma device in
> > dma_request_channel(). So we should in our filter function to validate
> > the device node with the device node specified by the dma_spec. Hope I
> > make things clear.
>
> But dma_request_channel() calls of_dma_request_slave_channel() which
> invokes of_dma_find_controller() why is it broken for you if that is the
> case..
No,the calling process should be:
dma_request_slave_channel()
--->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
----> dma_request_channel().
^ permalink raw reply
* Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Vinod Koul @ 2019-04-30 8:29 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <CAMz4ku+ctQrcR+6t1ouakeG3dbgL3qR8fz-Hft4s9FnxtFL1ng@mail.gmail.com>
On 30-04-19, 13:30, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 29-04-19, 20:20, Baolin Wang wrote:
> > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > From: Eric Long <eric.long@unisoc.com>
> > > > >
> > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > device validation in filter function to check if the correct controller
> > > > > to be requested.
> > > > >
> > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > ---
> > > > > drivers/dma/sprd-dma.c | 5 +++++
> > > > > 1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > index 0f92e60..9f99d4b 100644
> > > > > --- a/drivers/dma/sprd-dma.c
> > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > {
> > > > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > + struct of_phandle_args *dma_spec =
> > > > > + container_of(param, struct of_phandle_args, args[0]);
> > > > > u32 slave_id = *(u32 *)param;
> > > > >
> > > > > + if (chan->device->dev->of_node != dma_spec->np)
> > > >
> > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > be useless!
> > >
> > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > complicated than current solution. Since we need introduce one
> > > structure to save the node to validate in the filter function like
> > > below, which seems make things complicated. But if you still like to
> > > use of_dma_find_controller(), I can change to use it in next version.
> >
> > Sorry I should have clarified more..
> >
> > of_dma_find_controller() is called by xlate, so you already run this
> > check, so why use this :)
>
> The of_dma_find_controller() can save the requested device node into
> dma_spec, and in the of_dma_simple_xlate() function, it will call
> dma_request_channel() to request one channel, but it did not validate
> the device node to find the corresponding dma device in
> dma_request_channel(). So we should in our filter function to validate
> the device node with the device node specified by the dma_spec. Hope I
> make things clear.
But dma_request_channel() calls of_dma_request_slave_channel() which
invokes of_dma_find_controller() why is it broken for you if that is the
case..
--
~Vinod
^ permalink raw reply
* [4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Vinod Koul @ 2019-04-30 8:29 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On 30-04-19, 13:30, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 29-04-19, 20:20, Baolin Wang wrote:
> > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > > >
> > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > From: Eric Long <eric.long@unisoc.com>
> > > > >
> > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > device validation in filter function to check if the correct controller
> > > > > to be requested.
> > > > >
> > > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > > ---
> > > > > drivers/dma/sprd-dma.c | 5 +++++
> > > > > 1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > index 0f92e60..9f99d4b 100644
> > > > > --- a/drivers/dma/sprd-dma.c
> > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > {
> > > > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > + struct of_phandle_args *dma_spec =
> > > > > + container_of(param, struct of_phandle_args, args[0]);
> > > > > u32 slave_id = *(u32 *)param;
> > > > >
> > > > > + if (chan->device->dev->of_node != dma_spec->np)
> > > >
> > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > be useless!
> > >
> > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > complicated than current solution. Since we need introduce one
> > > structure to save the node to validate in the filter function like
> > > below, which seems make things complicated. But if you still like to
> > > use of_dma_find_controller(), I can change to use it in next version.
> >
> > Sorry I should have clarified more..
> >
> > of_dma_find_controller() is called by xlate, so you already run this
> > check, so why use this :)
>
> The of_dma_find_controller() can save the requested device node into
> dma_spec, and in the of_dma_simple_xlate() function, it will call
> dma_request_channel() to request one channel, but it did not validate
> the device node to find the corresponding dma device in
> dma_request_channel(). So we should in our filter function to validate
> the device node with the device node specified by the dma_spec. Hope I
> make things clear.
But dma_request_channel() calls of_dma_request_slave_channel() which
invokes of_dma_find_controller() why is it broken for you if that is the
case..
^ permalink raw reply
* Re: [PATCH] dmaengine: stm32-dma: fix residue calculation in stm32-dma
From: Vinod Koul @ 2019-04-30 8:22 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Dan Williams, Pierre-Yves MORDRET, linux-stm32, linux-kernel,
dmaengine
In-Reply-To: <26fa7710-76cb-e202-a367-c2e2408b6808@st.com>
On 29-04-19, 16:52, Arnaud Pouliquen wrote:
>
>
> On 4/29/19 7:13 AM, Vinod Koul wrote:
> > On 26-04-19, 15:41, Arnaud Pouliquen wrote:
> >>>> During residue calculation. the DMA can switch to the next sg. When
> >>>> this race condition occurs, the residue returned value is not valid.
> >>>> Indeed the position in the sg returned by the hardware is the position
> >>>> of the next sg, not the current sg.
> >>>> Solution is to check the sg after the calculation to verify it.
> >>>> If a transition is detected we consider that the DMA has switched to
> >>>> the beginning of next sg.
> >>>
> >>> Now, that sounds like duct tape. Why should we bother doing that.
> >>>
> >>> Also looking back at the stm32_dma_desc_residue() and calls to it from
> >>> stm32_dma_tx_status() am not sure we are doing the right thing
> >> Please, could you explain what you have in mind here?
> >
> > So when we call vchan_find_desc() that tells us if the descriptor is in
> > the issued queue or not.. Ideally it should not matter if we have one
> > or N descriptors issued to hardware.
> >
> > So why should you bother checking for next_sg.
> >
> >>> why are we looking at next_sg here, can you explain me that please
> >>
> >> This solution is similar to one implemented in the at_hdmac.c driver
> >> (atc_get_bytes_left function).
> >>
> >> Yes could be consider as a workaround for a hardware issue...
> >>
> >> In stm32 DMA Peripheral, we can register up to 2 sg descriptors (sg1 &
> >> sg2)in DMA registers, and use it in a cyclic mode (auto reload). This
> >> mode is mainly use for audio transfer initiated by an ALSA driver.
> >>
> >> >From hardware point of view the DMA transfers first block based on sg1,
> >> then it updates registers to prepare sg2 transfer, and then generates an
> >> IRQ to inform that it issues the next transfer (sg2).
> >>
> >> Then driver can update sg1 to prepare the third transfer...
> >>
> >> In parallel the client driver can requests status to get the residue to
> >> update internal pointer.
> >> The issue is in the race condition between the call of the
> >> device_tx_status ops and the update of the DMA register on sg switch.
> >
> > Sorry I do not agree! You are in stm32_dma_tx_status() hold the lock and
> > IRQs are disabled, so even if sg2 was loaded, you will not get an
> > interrupt and wont know. By looking at sg1 register you will see that
> > sg1 is telling you that it has finished and residue can be zero. That is
> > fine and correct to report.
> >
> > Most important thing here is that reside is for _requested_ descriptor
> > and not _current_ descriptor, so looking into sg2 doesnt not fit.
> >
> >> During a short time the hardware updated the registers containing the
> >> sg ID but not the transfer counter(SxNDTR). In this case there is a
> >> mismatch between the Sg ID and the associated transfer counter.
> >> So residue calculation is wrong.
> >> Idea of this patch is to perform the calculation and then to crosscheck
> >> that the hardware has not switched to the next sg during the
> >> calculation. The way to crosscheck is to compare the the sg ID before
> >> and after the calculation.
> >>
> >> I tested the solution to force a new recalculation but no real solution
> >> to trust the registers during this phase. In this case an approximation
> >> is to consider that the DMA is transferring the first bytes of the next sg.
> >> So we return the residue corresponding to the beginning of the next buffer.
> >
> > And that is wrong!. The argument is 'cookie' and you return residue for
> > that cookie.
> >
> > For example, if you have dma txn with cookie 1, 2, 3, 4 submitted, then currently HW
> > is processing cookie 2, then for tx_status on:
> > cookie 1: return DMA_COMPLETE, residue 0
> > cookie 2: return DMA_IN_PROGRESS, residue (read from HW)
> > cookie 3: return DMA_IN_PROGRESS, residue txn length
> > cookie 4: return DMA_IN_PROGRESS, residue txn length
> >
> > Thanks
> >
> I think i miss something in my explanation, as from my humble POV (not
> enough expert in DMA framework...) we only one cookie here as only one
> cyclic transfer...
> Regarding your answers it looks like my sg explanation are not clear and
> introduce confusions... Sorry for this, i was used sg for internal STM32
> DMA driver, not for the framework API itself.
>
> Let try retry to re-explain you the stm32 DMA cyclic mode management.
>
> STM32 STM32 hardware:
> -------------------
> (ref manual:
> https://www.st.com/content/ccc/resource/technical/document/reference_manual/group0/51/ba/9e/5e/78/5b/4b/dd/DM00327659/files/DM00327659.pdf/jcr:content/translations/en.DM00327659.pdf)
>
> The stm32 DMA supports cyclic mode using a hardware double
> buffer mode.
> In this double buffer, we can program up to 2 transfers. When one is
> completed, the DMA automatically switch on the other. This could be see
> as a hardware LLI with only 2 transfer descriptors.
> A hardware bit CT (current target) is used to determine the
> current transfer (CT = 0 or 1).
> A hardware NDT (num of data to transfer) counter can be read to
> determine DMA position in current transfer.
> An IRQ is generated when this CT bit is updated to allows driver to
> update the double buffer for the next transfer.
>
> On client side (a.e audio):
> -------------------------
> The client requests a cyclic transfer by calling
> stm32_dma_prep_dma_cyclic. For instance it can request the transfer of a
> buffer divided in 10 periods. In this case only one cookie submitted
> (right?).
>
> At stm32dma driver level these 10 periods are registered in an internal
> software table (desc->sg_req[]).As cyclic, the last sg_req point to the
> first one.
>
> So to be able to transfer the whole software table, we have to update
> the STM32 DMA double buffer at each end of transfer period.
> The filed chan->next_sg points to the next sg_req in the software table.
> that should be write in the STM32 DMA double buffer.
>
> Residue calculation:
> -------------------
> During a transfer we can get the position in a period thanks to the
> NDT(num of data to transfer) bit-field.
>
> So the calculation is :
> 1) Get the NDT field value
> 3) add the periods remaining in the desc->sg_req[] table.
>
> In parallel the STM32 DMA hardware updates the transfer buffer in 3 steps:
> 1) update CT register field.
> 2) Update NDT register field.
> 3) generate the IRQ (As you mention the IRQ is not treated during the
> device_tx_status as protected from interrupts).
>
> We are facing issue when computing the residue during the update of the
> CT and the NDT. The CT and NDT can as been updated ( both or only CT...)
> without driver context update (IRQ disabled).
> In this case we can point to the beginning of the current transfer(
> completed) instead of the next_transfer. This generates a residue error
> and for audio a time-stamp regression (so video freeze or audio plop).
>
> So the patch proposed consists in:
> 1) getting the current NDT value
> 2) reading CT and check that the hardware does not point to the next_sg.
> if yes:
> - CT has been updated by hardware but IRQ still not treated.
> - By default we consider the current_sg as completed, so we
> point to the beginning of the next_sg buffer.
>
> Hope that will help to clarify.
Yes that helps, maybe we should add these bits in code and changelog..
:)
And how does this impact non cyclic case where N descriptors maybe
issued. The driver seems to support non cyclic too...
Thanks
--
~Vinod
^ permalink raw reply
* dmaengine: stm32-dma: fix residue calculation in stm32-dma
From: Vinod Koul @ 2019-04-30 8:22 UTC (permalink / raw)
To: Arnaud Pouliquen
Cc: Dan Williams, Pierre-Yves MORDRET, linux-stm32, linux-kernel,
dmaengine
On 29-04-19, 16:52, Arnaud Pouliquen wrote:
>
>
> On 4/29/19 7:13 AM, Vinod Koul wrote:
> > On 26-04-19, 15:41, Arnaud Pouliquen wrote:
> >>>> During residue calculation. the DMA can switch to the next sg. When
> >>>> this race condition occurs, the residue returned value is not valid.
> >>>> Indeed the position in the sg returned by the hardware is the position
> >>>> of the next sg, not the current sg.
> >>>> Solution is to check the sg after the calculation to verify it.
> >>>> If a transition is detected we consider that the DMA has switched to
> >>>> the beginning of next sg.
> >>>
> >>> Now, that sounds like duct tape. Why should we bother doing that.
> >>>
> >>> Also looking back at the stm32_dma_desc_residue() and calls to it from
> >>> stm32_dma_tx_status() am not sure we are doing the right thing
> >> Please, could you explain what you have in mind here?
> >
> > So when we call vchan_find_desc() that tells us if the descriptor is in
> > the issued queue or not.. Ideally it should not matter if we have one
> > or N descriptors issued to hardware.
> >
> > So why should you bother checking for next_sg.
> >
> >>> why are we looking at next_sg here, can you explain me that please
> >>
> >> This solution is similar to one implemented in the at_hdmac.c driver
> >> (atc_get_bytes_left function).
> >>
> >> Yes could be consider as a workaround for a hardware issue...
> >>
> >> In stm32 DMA Peripheral, we can register up to 2 sg descriptors (sg1 &
> >> sg2)in DMA registers, and use it in a cyclic mode (auto reload). This
> >> mode is mainly use for audio transfer initiated by an ALSA driver.
> >>
> >> >From hardware point of view the DMA transfers first block based on sg1,
> >> then it updates registers to prepare sg2 transfer, and then generates an
> >> IRQ to inform that it issues the next transfer (sg2).
> >>
> >> Then driver can update sg1 to prepare the third transfer...
> >>
> >> In parallel the client driver can requests status to get the residue to
> >> update internal pointer.
> >> The issue is in the race condition between the call of the
> >> device_tx_status ops and the update of the DMA register on sg switch.
> >
> > Sorry I do not agree! You are in stm32_dma_tx_status() hold the lock and
> > IRQs are disabled, so even if sg2 was loaded, you will not get an
> > interrupt and wont know. By looking at sg1 register you will see that
> > sg1 is telling you that it has finished and residue can be zero. That is
> > fine and correct to report.
> >
> > Most important thing here is that reside is for _requested_ descriptor
> > and not _current_ descriptor, so looking into sg2 doesnt not fit.
> >
> >> During a short time the hardware updated the registers containing the
> >> sg ID but not the transfer counter(SxNDTR). In this case there is a
> >> mismatch between the Sg ID and the associated transfer counter.
> >> So residue calculation is wrong.
> >> Idea of this patch is to perform the calculation and then to crosscheck
> >> that the hardware has not switched to the next sg during the
> >> calculation. The way to crosscheck is to compare the the sg ID before
> >> and after the calculation.
> >>
> >> I tested the solution to force a new recalculation but no real solution
> >> to trust the registers during this phase. In this case an approximation
> >> is to consider that the DMA is transferring the first bytes of the next sg.
> >> So we return the residue corresponding to the beginning of the next buffer.
> >
> > And that is wrong!. The argument is 'cookie' and you return residue for
> > that cookie.
> >
> > For example, if you have dma txn with cookie 1, 2, 3, 4 submitted, then currently HW
> > is processing cookie 2, then for tx_status on:
> > cookie 1: return DMA_COMPLETE, residue 0
> > cookie 2: return DMA_IN_PROGRESS, residue (read from HW)
> > cookie 3: return DMA_IN_PROGRESS, residue txn length
> > cookie 4: return DMA_IN_PROGRESS, residue txn length
> >
> > Thanks
> >
> I think i miss something in my explanation, as from my humble POV (not
> enough expert in DMA framework...) we only one cookie here as only one
> cyclic transfer...
> Regarding your answers it looks like my sg explanation are not clear and
> introduce confusions... Sorry for this, i was used sg for internal STM32
> DMA driver, not for the framework API itself.
>
> Let try retry to re-explain you the stm32 DMA cyclic mode management.
>
> STM32 STM32 hardware:
> -------------------
> (ref manual:
> https://www.st.com/content/ccc/resource/technical/document/reference_manual/group0/51/ba/9e/5e/78/5b/4b/dd/DM00327659/files/DM00327659.pdf/jcr:content/translations/en.DM00327659.pdf)
>
> The stm32 DMA supports cyclic mode using a hardware double
> buffer mode.
> In this double buffer, we can program up to 2 transfers. When one is
> completed, the DMA automatically switch on the other. This could be see
> as a hardware LLI with only 2 transfer descriptors.
> A hardware bit CT (current target) is used to determine the
> current transfer (CT = 0 or 1).
> A hardware NDT (num of data to transfer) counter can be read to
> determine DMA position in current transfer.
> An IRQ is generated when this CT bit is updated to allows driver to
> update the double buffer for the next transfer.
>
> On client side (a.e audio):
> -------------------------
> The client requests a cyclic transfer by calling
> stm32_dma_prep_dma_cyclic. For instance it can request the transfer of a
> buffer divided in 10 periods. In this case only one cookie submitted
> (right?).
>
> At stm32dma driver level these 10 periods are registered in an internal
> software table (desc->sg_req[]).As cyclic, the last sg_req point to the
> first one.
>
> So to be able to transfer the whole software table, we have to update
> the STM32 DMA double buffer at each end of transfer period.
> The filed chan->next_sg points to the next sg_req in the software table.
> that should be write in the STM32 DMA double buffer.
>
> Residue calculation:
> -------------------
> During a transfer we can get the position in a period thanks to the
> NDT(num of data to transfer) bit-field.
>
> So the calculation is :
> 1) Get the NDT field value
> 3) add the periods remaining in the desc->sg_req[] table.
>
> In parallel the STM32 DMA hardware updates the transfer buffer in 3 steps:
> 1) update CT register field.
> 2) Update NDT register field.
> 3) generate the IRQ (As you mention the IRQ is not treated during the
> device_tx_status as protected from interrupts).
>
> We are facing issue when computing the residue during the update of the
> CT and the NDT. The CT and NDT can as been updated ( both or only CT...)
> without driver context update (IRQ disabled).
> In this case we can point to the beginning of the current transfer(
> completed) instead of the next_transfer. This generates a residue error
> and for audio a time-stamp regression (so video freeze or audio plop).
>
> So the patch proposed consists in:
> 1) getting the current NDT value
> 2) reading CT and check that the hardware does not point to the next_sg.
> if yes:
> - CT has been updated by hardware but IRQ still not treated.
> - By default we consider the current_sg as completed, so we
> point to the beginning of the next_sg buffer.
>
> Hope that will help to clarify.
Yes that helps, maybe we should add these bits in code and changelog..
:)
And how does this impact non cyclic case where N descriptors maybe
issued. The driver seems to support non cyclic too...
Thanks
^ permalink raw reply
* Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Baolin Wang @ 2019-04-30 5:37 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <20190429141009.GO3845@vkoul-mobl.Dlink>
On Mon, 29 Apr 2019 at 22:10, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 29-04-19, 20:11, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
> > > On 15-04-19, 20:15, Baolin Wang wrote:
>
> > > > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > > > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > > > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > > > val |= SPRD_DMA_GLB_2STAGE_EN;
> > > > + if (schan->int_type != SPRD_DMA_NO_INT)
> > >
> > > Who configure int_type?
> >
> > The int_type is configured through the flags of
> > sprd_dma_prep_slave_sg() by users, see:
> > https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
>
> Please use DMA_PREP_INTERRUPT flag instead!
We can not use DMA_PREP_INTERRUPT flag, since we have some Spreadtrum
specific DMA interrupt flags configured by users, which I think we
have made a consensus before. See:
https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L105
--
Baolin Wang
Best Regards
^ permalink raw reply
* [7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Baolin Wang @ 2019-04-30 5:37 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On Mon, 29 Apr 2019 at 22:10, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 29-04-19, 20:11, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
> > > On 15-04-19, 20:15, Baolin Wang wrote:
>
> > > > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > > > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > > > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > > > val |= SPRD_DMA_GLB_2STAGE_EN;
> > > > + if (schan->int_type != SPRD_DMA_NO_INT)
> > >
> > > Who configure int_type?
> >
> > The int_type is configured through the flags of
> > sprd_dma_prep_slave_sg() by users, see:
> > https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
>
> Please use DMA_PREP_INTERRUPT flag instead!
We can not use DMA_PREP_INTERRUPT flag, since we have some Spreadtrum
specific DMA interrupt flags configured by users, which I think we
have made a consensus before. See:
https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L105
^ permalink raw reply
* Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Baolin Wang @ 2019-04-30 5:30 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <20190429140537.GN3845@vkoul-mobl.Dlink>
On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 29-04-19, 20:20, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > From: Eric Long <eric.long@unisoc.com>
> > > >
> > > > Since we can support multiple DMA engine controllers, we should add
> > > > device validation in filter function to check if the correct controller
> > > > to be requested.
> > > >
> > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > ---
> > > > drivers/dma/sprd-dma.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > index 0f92e60..9f99d4b 100644
> > > > --- a/drivers/dma/sprd-dma.c
> > > > +++ b/drivers/dma/sprd-dma.c
> > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > {
> > > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > + struct of_phandle_args *dma_spec =
> > > > + container_of(param, struct of_phandle_args, args[0]);
> > > > u32 slave_id = *(u32 *)param;
> > > >
> > > > + if (chan->device->dev->of_node != dma_spec->np)
> > >
> > > Are you not using of_dma_find_controller() that does this, so this would
> > > be useless!
> >
> > Yes, we can use of_dma_find_controller(), but that will be a little
> > complicated than current solution. Since we need introduce one
> > structure to save the node to validate in the filter function like
> > below, which seems make things complicated. But if you still like to
> > use of_dma_find_controller(), I can change to use it in next version.
>
> Sorry I should have clarified more..
>
> of_dma_find_controller() is called by xlate, so you already run this
> check, so why use this :)
The of_dma_find_controller() can save the requested device node into
dma_spec, and in the of_dma_simple_xlate() function, it will call
dma_request_channel() to request one channel, but it did not validate
the device node to find the corresponding dma device in
dma_request_channel(). So we should in our filter function to validate
the device node with the device node specified by the dma_spec. Hope I
make things clear.
--
Baolin Wang
Best Regards
^ permalink raw reply
* [4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Baolin Wang @ 2019-04-30 5:30 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 29-04-19, 20:20, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > From: Eric Long <eric.long@unisoc.com>
> > > >
> > > > Since we can support multiple DMA engine controllers, we should add
> > > > device validation in filter function to check if the correct controller
> > > > to be requested.
> > > >
> > > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > > ---
> > > > drivers/dma/sprd-dma.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > index 0f92e60..9f99d4b 100644
> > > > --- a/drivers/dma/sprd-dma.c
> > > > +++ b/drivers/dma/sprd-dma.c
> > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > {
> > > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > + struct of_phandle_args *dma_spec =
> > > > + container_of(param, struct of_phandle_args, args[0]);
> > > > u32 slave_id = *(u32 *)param;
> > > >
> > > > + if (chan->device->dev->of_node != dma_spec->np)
> > >
> > > Are you not using of_dma_find_controller() that does this, so this would
> > > be useless!
> >
> > Yes, we can use of_dma_find_controller(), but that will be a little
> > complicated than current solution. Since we need introduce one
> > structure to save the node to validate in the filter function like
> > below, which seems make things complicated. But if you still like to
> > use of_dma_find_controller(), I can change to use it in next version.
>
> Sorry I should have clarified more..
>
> of_dma_find_controller() is called by xlate, so you already run this
> check, so why use this :)
The of_dma_find_controller() can save the requested device node into
dma_spec, and in the of_dma_simple_xlate() function, it will call
dma_request_channel() to request one channel, but it did not validate
the device node to find the corresponding dma device in
dma_request_channel(). So we should in our filter function to validate
the device node with the device node specified by the dma_spec. Hope I
make things clear.
^ permalink raw reply
* Re: [PATCH] dmaengine: stm32-dma: fix residue calculation in stm32-dma
From: Arnaud Pouliquen @ 2019-04-29 14:52 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, Pierre-Yves MORDRET, linux-stm32, linux-kernel,
dmaengine
In-Reply-To: <20190429051310.GC3845@vkoul-mobl.Dlink>
On 4/29/19 7:13 AM, Vinod Koul wrote:
> On 26-04-19, 15:41, Arnaud Pouliquen wrote:
>>>> During residue calculation. the DMA can switch to the next sg. When
>>>> this race condition occurs, the residue returned value is not valid.
>>>> Indeed the position in the sg returned by the hardware is the position
>>>> of the next sg, not the current sg.
>>>> Solution is to check the sg after the calculation to verify it.
>>>> If a transition is detected we consider that the DMA has switched to
>>>> the beginning of next sg.
>>>
>>> Now, that sounds like duct tape. Why should we bother doing that.
>>>
>>> Also looking back at the stm32_dma_desc_residue() and calls to it from
>>> stm32_dma_tx_status() am not sure we are doing the right thing
>> Please, could you explain what you have in mind here?
>
> So when we call vchan_find_desc() that tells us if the descriptor is in
> the issued queue or not.. Ideally it should not matter if we have one
> or N descriptors issued to hardware.
>
> So why should you bother checking for next_sg.
>
>>> why are we looking at next_sg here, can you explain me that please
>>
>> This solution is similar to one implemented in the at_hdmac.c driver
>> (atc_get_bytes_left function).
>>
>> Yes could be consider as a workaround for a hardware issue...
>>
>> In stm32 DMA Peripheral, we can register up to 2 sg descriptors (sg1 &
>> sg2)in DMA registers, and use it in a cyclic mode (auto reload). This
>> mode is mainly use for audio transfer initiated by an ALSA driver.
>>
>> >From hardware point of view the DMA transfers first block based on sg1,
>> then it updates registers to prepare sg2 transfer, and then generates an
>> IRQ to inform that it issues the next transfer (sg2).
>>
>> Then driver can update sg1 to prepare the third transfer...
>>
>> In parallel the client driver can requests status to get the residue to
>> update internal pointer.
>> The issue is in the race condition between the call of the
>> device_tx_status ops and the update of the DMA register on sg switch.
>
> Sorry I do not agree! You are in stm32_dma_tx_status() hold the lock and
> IRQs are disabled, so even if sg2 was loaded, you will not get an
> interrupt and wont know. By looking at sg1 register you will see that
> sg1 is telling you that it has finished and residue can be zero. That is
> fine and correct to report.
>
> Most important thing here is that reside is for _requested_ descriptor
> and not _current_ descriptor, so looking into sg2 doesnt not fit.
>
>> During a short time the hardware updated the registers containing the
>> sg ID but not the transfer counter(SxNDTR). In this case there is a
>> mismatch between the Sg ID and the associated transfer counter.
>> So residue calculation is wrong.
>> Idea of this patch is to perform the calculation and then to crosscheck
>> that the hardware has not switched to the next sg during the
>> calculation. The way to crosscheck is to compare the the sg ID before
>> and after the calculation.
>>
>> I tested the solution to force a new recalculation but no real solution
>> to trust the registers during this phase. In this case an approximation
>> is to consider that the DMA is transferring the first bytes of the next sg.
>> So we return the residue corresponding to the beginning of the next buffer.
>
> And that is wrong!. The argument is 'cookie' and you return residue for
> that cookie.
>
> For example, if you have dma txn with cookie 1, 2, 3, 4 submitted, then currently HW
> is processing cookie 2, then for tx_status on:
> cookie 1: return DMA_COMPLETE, residue 0
> cookie 2: return DMA_IN_PROGRESS, residue (read from HW)
> cookie 3: return DMA_IN_PROGRESS, residue txn length
> cookie 4: return DMA_IN_PROGRESS, residue txn length
>
> Thanks
>
I think i miss something in my explanation, as from my humble POV (not
enough expert in DMA framework...) we only one cookie here as only one
cyclic transfer...
Regarding your answers it looks like my sg explanation are not clear and
introduce confusions... Sorry for this, i was used sg for internal STM32
DMA driver, not for the framework API itself.
Let try retry to re-explain you the stm32 DMA cyclic mode management.
STM32 STM32 hardware:
-------------------
(ref manual:
https://www.st.com/content/ccc/resource/technical/document/reference_manual/group0/51/ba/9e/5e/78/5b/4b/dd/DM00327659/files/DM00327659.pdf/jcr:content/translations/en.DM00327659.pdf)
The stm32 DMA supports cyclic mode using a hardware double
buffer mode.
In this double buffer, we can program up to 2 transfers. When one is
completed, the DMA automatically switch on the other. This could be see
as a hardware LLI with only 2 transfer descriptors.
A hardware bit CT (current target) is used to determine the
current transfer (CT = 0 or 1).
A hardware NDT (num of data to transfer) counter can be read to
determine DMA position in current transfer.
An IRQ is generated when this CT bit is updated to allows driver to
update the double buffer for the next transfer.
On client side (a.e audio):
-------------------------
The client requests a cyclic transfer by calling
stm32_dma_prep_dma_cyclic. For instance it can request the transfer of a
buffer divided in 10 periods. In this case only one cookie submitted
(right?).
At stm32dma driver level these 10 periods are registered in an internal
software table (desc->sg_req[]).As cyclic, the last sg_req point to the
first one.
So to be able to transfer the whole software table, we have to update
the STM32 DMA double buffer at each end of transfer period.
The filed chan->next_sg points to the next sg_req in the software table.
that should be write in the STM32 DMA double buffer.
Residue calculation:
-------------------
During a transfer we can get the position in a period thanks to the
NDT(num of data to transfer) bit-field.
So the calculation is :
1) Get the NDT field value
3) add the periods remaining in the desc->sg_req[] table.
In parallel the STM32 DMA hardware updates the transfer buffer in 3 steps:
1) update CT register field.
2) Update NDT register field.
3) generate the IRQ (As you mention the IRQ is not treated during the
device_tx_status as protected from interrupts).
We are facing issue when computing the residue during the update of the
CT and the NDT. The CT and NDT can as been updated ( both or only CT...)
without driver context update (IRQ disabled).
In this case we can point to the beginning of the current transfer(
completed) instead of the next_transfer. This generates a residue error
and for audio a time-stamp regression (so video freeze or audio plop).
So the patch proposed consists in:
1) getting the current NDT value
2) reading CT and check that the hardware does not point to the next_sg.
if yes:
- CT has been updated by hardware but IRQ still not treated.
- By default we consider the current_sg as completed, so we
point to the beginning of the next_sg buffer.
Hope that will help to clarify.
Thanks
arnaud
^ permalink raw reply
* dmaengine: stm32-dma: fix residue calculation in stm32-dma
From: Arnaud Pouliquen @ 2019-04-29 14:52 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, Pierre-Yves MORDRET, linux-stm32, linux-kernel,
dmaengine
On 4/29/19 7:13 AM, Vinod Koul wrote:
> On 26-04-19, 15:41, Arnaud Pouliquen wrote:
>>>> During residue calculation. the DMA can switch to the next sg. When
>>>> this race condition occurs, the residue returned value is not valid.
>>>> Indeed the position in the sg returned by the hardware is the position
>>>> of the next sg, not the current sg.
>>>> Solution is to check the sg after the calculation to verify it.
>>>> If a transition is detected we consider that the DMA has switched to
>>>> the beginning of next sg.
>>>
>>> Now, that sounds like duct tape. Why should we bother doing that.
>>>
>>> Also looking back at the stm32_dma_desc_residue() and calls to it from
>>> stm32_dma_tx_status() am not sure we are doing the right thing
>> Please, could you explain what you have in mind here?
>
> So when we call vchan_find_desc() that tells us if the descriptor is in
> the issued queue or not.. Ideally it should not matter if we have one
> or N descriptors issued to hardware.
>
> So why should you bother checking for next_sg.
>
>>> why are we looking at next_sg here, can you explain me that please
>>
>> This solution is similar to one implemented in the at_hdmac.c driver
>> (atc_get_bytes_left function).
>>
>> Yes could be consider as a workaround for a hardware issue...
>>
>> In stm32 DMA Peripheral, we can register up to 2 sg descriptors (sg1 &
>> sg2)in DMA registers, and use it in a cyclic mode (auto reload). This
>> mode is mainly use for audio transfer initiated by an ALSA driver.
>>
>> >From hardware point of view the DMA transfers first block based on sg1,
>> then it updates registers to prepare sg2 transfer, and then generates an
>> IRQ to inform that it issues the next transfer (sg2).
>>
>> Then driver can update sg1 to prepare the third transfer...
>>
>> In parallel the client driver can requests status to get the residue to
>> update internal pointer.
>> The issue is in the race condition between the call of the
>> device_tx_status ops and the update of the DMA register on sg switch.
>
> Sorry I do not agree! You are in stm32_dma_tx_status() hold the lock and
> IRQs are disabled, so even if sg2 was loaded, you will not get an
> interrupt and wont know. By looking at sg1 register you will see that
> sg1 is telling you that it has finished and residue can be zero. That is
> fine and correct to report.
>
> Most important thing here is that reside is for _requested_ descriptor
> and not _current_ descriptor, so looking into sg2 doesnt not fit.
>
>> During a short time the hardware updated the registers containing the
>> sg ID but not the transfer counter(SxNDTR). In this case there is a
>> mismatch between the Sg ID and the associated transfer counter.
>> So residue calculation is wrong.
>> Idea of this patch is to perform the calculation and then to crosscheck
>> that the hardware has not switched to the next sg during the
>> calculation. The way to crosscheck is to compare the the sg ID before
>> and after the calculation.
>>
>> I tested the solution to force a new recalculation but no real solution
>> to trust the registers during this phase. In this case an approximation
>> is to consider that the DMA is transferring the first bytes of the next sg.
>> So we return the residue corresponding to the beginning of the next buffer.
>
> And that is wrong!. The argument is 'cookie' and you return residue for
> that cookie.
>
> For example, if you have dma txn with cookie 1, 2, 3, 4 submitted, then currently HW
> is processing cookie 2, then for tx_status on:
> cookie 1: return DMA_COMPLETE, residue 0
> cookie 2: return DMA_IN_PROGRESS, residue (read from HW)
> cookie 3: return DMA_IN_PROGRESS, residue txn length
> cookie 4: return DMA_IN_PROGRESS, residue txn length
>
> Thanks
>
I think i miss something in my explanation, as from my humble POV (not
enough expert in DMA framework...) we only one cookie here as only one
cyclic transfer...
Regarding your answers it looks like my sg explanation are not clear and
introduce confusions... Sorry for this, i was used sg for internal STM32
DMA driver, not for the framework API itself.
Let try retry to re-explain you the stm32 DMA cyclic mode management.
STM32 STM32 hardware:
-------------------
(ref manual:
https://www.st.com/content/ccc/resource/technical/document/reference_manual/group0/51/ba/9e/5e/78/5b/4b/dd/DM00327659/files/DM00327659.pdf/jcr:content/translations/en.DM00327659.pdf)
The stm32 DMA supports cyclic mode using a hardware double
buffer mode.
In this double buffer, we can program up to 2 transfers. When one is
completed, the DMA automatically switch on the other. This could be see
as a hardware LLI with only 2 transfer descriptors.
A hardware bit CT (current target) is used to determine the
current transfer (CT = 0 or 1).
A hardware NDT (num of data to transfer) counter can be read to
determine DMA position in current transfer.
An IRQ is generated when this CT bit is updated to allows driver to
update the double buffer for the next transfer.
On client side (a.e audio):
-------------------------
The client requests a cyclic transfer by calling
stm32_dma_prep_dma_cyclic. For instance it can request the transfer of a
buffer divided in 10 periods. In this case only one cookie submitted
(right?).
At stm32dma driver level these 10 periods are registered in an internal
software table (desc->sg_req[]).As cyclic, the last sg_req point to the
first one.
So to be able to transfer the whole software table, we have to update
the STM32 DMA double buffer at each end of transfer period.
The filed chan->next_sg points to the next sg_req in the software table.
that should be write in the STM32 DMA double buffer.
Residue calculation:
-------------------
During a transfer we can get the position in a period thanks to the
NDT(num of data to transfer) bit-field.
So the calculation is :
1) Get the NDT field value
3) add the periods remaining in the desc->sg_req[] table.
In parallel the STM32 DMA hardware updates the transfer buffer in 3 steps:
1) update CT register field.
2) Update NDT register field.
3) generate the IRQ (As you mention the IRQ is not treated during the
device_tx_status as protected from interrupts).
We are facing issue when computing the residue during the update of the
CT and the NDT. The CT and NDT can as been updated ( both or only CT...)
without driver context update (IRQ disabled).
In this case we can point to the beginning of the current transfer(
completed) instead of the next_transfer. This generates a residue error
and for audio a time-stamp regression (so video freeze or audio plop).
So the patch proposed consists in:
1) getting the current NDT value
2) reading CT and check that the hardware does not point to the next_sg.
if yes:
- CT has been updated by hardware but IRQ still not treated.
- By default we consider the current_sg as completed, so we
point to the beginning of the next_sg buffer.
Hope that will help to clarify.
Thanks
arnaud
^ permalink raw reply
* Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Vinod Koul @ 2019-04-29 14:10 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <CAMz4kuJB2+6HziyDep4ctfmjFYpmZ-v_vrFQsJ9tHvwYzSJeKA@mail.gmail.com>
On 29-04-19, 20:11, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
> > On 15-04-19, 20:15, Baolin Wang wrote:
> > > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > > val |= SPRD_DMA_GLB_2STAGE_EN;
> > > + if (schan->int_type != SPRD_DMA_NO_INT)
> >
> > Who configure int_type?
>
> The int_type is configured through the flags of
> sprd_dma_prep_slave_sg() by users, see:
> https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
Please use DMA_PREP_INTERRUPT flag instead!
--
~Vinod
^ permalink raw reply
* [7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Vinod Koul @ 2019-04-29 14:10 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On 29-04-19, 20:11, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
> > On 15-04-19, 20:15, Baolin Wang wrote:
> > > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > > val |= SPRD_DMA_GLB_2STAGE_EN;
> > > + if (schan->int_type != SPRD_DMA_NO_INT)
> >
> > Who configure int_type?
>
> The int_type is configured through the flags of
> sprd_dma_prep_slave_sg() by users, see:
> https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
Please use DMA_PREP_INTERRUPT flag instead!
^ permalink raw reply
* Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Vinod Koul @ 2019-04-29 14:05 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <CAMz4kuLf4wgr4Js3xH1aQVc4c2XK1Oq2TnsUq=NSowQUq5ZN5g@mail.gmail.com>
On 29-04-19, 20:20, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > From: Eric Long <eric.long@unisoc.com>
> > >
> > > Since we can support multiple DMA engine controllers, we should add
> > > device validation in filter function to check if the correct controller
> > > to be requested.
> > >
> > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > > drivers/dma/sprd-dma.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 0f92e60..9f99d4b 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > {
> > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > + struct of_phandle_args *dma_spec =
> > > + container_of(param, struct of_phandle_args, args[0]);
> > > u32 slave_id = *(u32 *)param;
> > >
> > > + if (chan->device->dev->of_node != dma_spec->np)
> >
> > Are you not using of_dma_find_controller() that does this, so this would
> > be useless!
>
> Yes, we can use of_dma_find_controller(), but that will be a little
> complicated than current solution. Since we need introduce one
> structure to save the node to validate in the filter function like
> below, which seems make things complicated. But if you still like to
> use of_dma_find_controller(), I can change to use it in next version.
Sorry I should have clarified more..
of_dma_find_controller() is called by xlate, so you already run this
check, so why use this :)
--
~Vinod
^ permalink raw reply
* [4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Vinod Koul @ 2019-04-29 14:05 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On 29-04-19, 20:20, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > From: Eric Long <eric.long@unisoc.com>
> > >
> > > Since we can support multiple DMA engine controllers, we should add
> > > device validation in filter function to check if the correct controller
> > > to be requested.
> > >
> > > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > > drivers/dma/sprd-dma.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 0f92e60..9f99d4b 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > {
> > > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > + struct of_phandle_args *dma_spec =
> > > + container_of(param, struct of_phandle_args, args[0]);
> > > u32 slave_id = *(u32 *)param;
> > >
> > > + if (chan->device->dev->of_node != dma_spec->np)
> >
> > Are you not using of_dma_find_controller() that does this, so this would
> > be useless!
>
> Yes, we can use of_dma_find_controller(), but that will be a little
> complicated than current solution. Since we need introduce one
> structure to save the node to validate in the filter function like
> below, which seems make things complicated. But if you still like to
> use of_dma_find_controller(), I can change to use it in next version.
Sorry I should have clarified more..
of_dma_find_controller() is called by xlate, so you already run this
check, so why use this :)
^ permalink raw reply
* Re: [PATCH 3/4] dt-bindings: dma: uart: rename binding
From: Rob Herring @ 2019-04-29 12:48 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Nicolas Boichat, Matthias Brugger, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Zhenbao Liu, Long Cheng
In-Reply-To: <1556336193-15198-4-git-send-email-long.cheng@mediatek.com>
On Sat, 27 Apr 2019 11:36:32 +0800, Long Cheng wrote:
> The filename matches mtk-uart-apdma.c.
> So using "mtk-uart-apdma.txt" should be better.
> And add some property.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> .../devicetree/bindings/dma/8250_mtk_dma.txt | 33 ------------
> .../devicetree/bindings/dma/mtk-uart-apdma.txt | 55 ++++++++++++++++++++
> 2 files changed, 55 insertions(+), 33 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
> create mode 100644 Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
^ permalink raw reply
* [3/4] dt-bindings: dma: uart: rename binding
From: Rob Herring @ 2019-04-29 12:48 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Nicolas Boichat, Matthias Brugger, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Zhenbao Liu
On Sat, 27 Apr 2019 11:36:32 +0800, Long Cheng wrote:
> The filename matches mtk-uart-apdma.c.
> So using "mtk-uart-apdma.txt" should be better.
> And add some property.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> .../devicetree/bindings/dma/8250_mtk_dma.txt | 33 ------------
> .../devicetree/bindings/dma/mtk-uart-apdma.txt | 55 ++++++++++++++++++++
> 2 files changed, 55 insertions(+), 33 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
> create mode 100644 Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
^ permalink raw reply
* Re: [PATCH 4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Baolin Wang @ 2019-04-29 12:20 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <20190429115723.GK3845@vkoul-mobl.Dlink>
On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > From: Eric Long <eric.long@unisoc.com>
> >
> > Since we can support multiple DMA engine controllers, we should add
> > device validation in filter function to check if the correct controller
> > to be requested.
> >
> > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > drivers/dma/sprd-dma.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 0f92e60..9f99d4b 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > {
> > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > + struct of_phandle_args *dma_spec =
> > + container_of(param, struct of_phandle_args, args[0]);
> > u32 slave_id = *(u32 *)param;
> >
> > + if (chan->device->dev->of_node != dma_spec->np)
>
> Are you not using of_dma_find_controller() that does this, so this would
> be useless!
Yes, we can use of_dma_find_controller(), but that will be a little
complicated than current solution. Since we need introduce one
structure to save the node to validate in the filter function like
below, which seems make things complicated. But if you still like to
use of_dma_find_controller(), I can change to use it in next version.
Thank.
struct sprd_dma_filter_param {
struct device_node *np;
};
static struct dma_chan* sprd_dma_xlate(struct of_phandle_args
*dma_spec, struct of_dma *of_dma)
{
param.np = dma_spec->node;
return dma_request_channel(xxx);
}
of_dma_controller_register(np, sprd_dma_xlate, sdev);
--
Baolin Wang
Best Regards
^ permalink raw reply
* [4/7] dmaengine: sprd: Add device validation to support multiple controllers
From: Baolin Wang @ 2019-04-29 12:20 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > From: Eric Long <eric.long@unisoc.com>
> >
> > Since we can support multiple DMA engine controllers, we should add
> > device validation in filter function to check if the correct controller
> > to be requested.
> >
> > Signed-off-by: Eric Long <eric.long@unisoc.com>
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > drivers/dma/sprd-dma.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 0f92e60..9f99d4b 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > {
> > struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > + struct of_phandle_args *dma_spec =
> > + container_of(param, struct of_phandle_args, args[0]);
> > u32 slave_id = *(u32 *)param;
> >
> > + if (chan->device->dev->of_node != dma_spec->np)
>
> Are you not using of_dma_find_controller() that does this, so this would
> be useless!
Yes, we can use of_dma_find_controller(), but that will be a little
complicated than current solution. Since we need introduce one
structure to save the node to validate in the filter function like
below, which seems make things complicated. But if you still like to
use of_dma_find_controller(), I can change to use it in next version.
Thank.
struct sprd_dma_filter_param {
struct device_node *np;
};
static struct dma_chan* sprd_dma_xlate(struct of_phandle_args
*dma_spec, struct of_dma *of_dma)
{
param.np = dma_spec->node;
return dma_request_channel(xxx);
}
of_dma_controller_register(np, sprd_dma_xlate, sdev);
^ permalink raw reply
* Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Baolin Wang @ 2019-04-29 12:11 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <20190429120108.GL3845@vkoul-mobl.Dlink>
On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:15, Baolin Wang wrote:
> > For 2-stage transfer, some users like Audio still need transaction interrupt
> > to notify when the 2-stage transfer is completed. Thus we should enable
> > 2-stage transfer interrupt to support this feature.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > drivers/dma/sprd-dma.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index cc9c24d..4c18f44 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -62,6 +62,8 @@
> > /* SPRD_DMA_GLB_2STAGE_GRP register definition */
> > #define SPRD_DMA_GLB_2STAGE_EN BIT(24)
> > #define SPRD_DMA_GLB_CHN_INT_MASK GENMASK(23, 20)
> > +#define SPRD_DMA_GLB_DEST_INT BIT(22)
> > +#define SPRD_DMA_GLB_SRC_INT BIT(20)
> > #define SPRD_DMA_GLB_LIST_DONE_TRG BIT(19)
> > #define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
> > #define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
> > @@ -135,6 +137,7 @@
> > /* define DMA channel mode & trigger mode mask */
> > #define SPRD_DMA_CHN_MODE_MASK GENMASK(7, 0)
> > #define SPRD_DMA_TRG_MODE_MASK GENMASK(7, 0)
> > +#define SPRD_DMA_INT_TYPE_MASK GENMASK(7, 0)
> >
> > /* define the DMA transfer step type */
> > #define SPRD_DMA_NONE_STEP 0
> > @@ -190,6 +193,7 @@ struct sprd_dma_chn {
> > u32 dev_id;
> > enum sprd_dma_chn_mode chn_mode;
> > enum sprd_dma_trg_mode trg_mode;
> > + enum sprd_dma_int_type int_type;
> > struct sprd_dma_desc *cur_desc;
> > };
> >
> > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
>
> Who configure int_type?
The int_type is configured through the flags of
sprd_dma_prep_slave_sg() by users, see:
https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
>
> > + val |= SPRD_DMA_GLB_SRC_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> > break;
> >
> > @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
> > + val |= SPRD_DMA_GLB_SRC_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> > break;
> >
> > @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > SPRD_DMA_GLB_DEST_CHN_MASK;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
> > + val |= SPRD_DMA_GLB_DEST_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> > break;
> >
> > @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > SPRD_DMA_GLB_DEST_CHN_MASK;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
> > + val |= SPRD_DMA_GLB_DEST_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> > break;
> >
> > @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
> > schan->linklist.virt_addr = 0;
> > }
> >
> > - /* Set channel mode and trigger mode for 2-stage transfer */
> > + /*
> > + * Set channel mode, interrupt mode and trigger mode for 2-stage
> > + * transfer.
> > + */
> > schan->chn_mode =
> > (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
> > schan->trg_mode =
> > (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> > + schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
> >
> > sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> > if (!sdesc)
> > --
> > 1.7.9.5
>
> --
> ~Vinod
--
Baolin Wang
Best Regards
^ permalink raw reply
* [7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Baolin Wang @ 2019-04-29 12:11 UTC (permalink / raw)
To: Vinod Koul
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On Mon, 29 Apr 2019 at 20:01, Vinod Koul <vkoul@kernel.org> wrote:
>
> On 15-04-19, 20:15, Baolin Wang wrote:
> > For 2-stage transfer, some users like Audio still need transaction interrupt
> > to notify when the 2-stage transfer is completed. Thus we should enable
> > 2-stage transfer interrupt to support this feature.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > drivers/dma/sprd-dma.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index cc9c24d..4c18f44 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -62,6 +62,8 @@
> > /* SPRD_DMA_GLB_2STAGE_GRP register definition */
> > #define SPRD_DMA_GLB_2STAGE_EN BIT(24)
> > #define SPRD_DMA_GLB_CHN_INT_MASK GENMASK(23, 20)
> > +#define SPRD_DMA_GLB_DEST_INT BIT(22)
> > +#define SPRD_DMA_GLB_SRC_INT BIT(20)
> > #define SPRD_DMA_GLB_LIST_DONE_TRG BIT(19)
> > #define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
> > #define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
> > @@ -135,6 +137,7 @@
> > /* define DMA channel mode & trigger mode mask */
> > #define SPRD_DMA_CHN_MODE_MASK GENMASK(7, 0)
> > #define SPRD_DMA_TRG_MODE_MASK GENMASK(7, 0)
> > +#define SPRD_DMA_INT_TYPE_MASK GENMASK(7, 0)
> >
> > /* define the DMA transfer step type */
> > #define SPRD_DMA_NONE_STEP 0
> > @@ -190,6 +193,7 @@ struct sprd_dma_chn {
> > u32 dev_id;
> > enum sprd_dma_chn_mode chn_mode;
> > enum sprd_dma_trg_mode trg_mode;
> > + enum sprd_dma_int_type int_type;
> > struct sprd_dma_desc *cur_desc;
> > };
> >
> > @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
>
> Who configure int_type?
The int_type is configured through the flags of
sprd_dma_prep_slave_sg() by users, see:
https://elixir.bootlin.com/linux/v5.1-rc6/source/include/linux/dma/sprd-dma.h#L9
>
> > + val |= SPRD_DMA_GLB_SRC_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> > break;
> >
> > @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
> > + val |= SPRD_DMA_GLB_SRC_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> > break;
> >
> > @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > SPRD_DMA_GLB_DEST_CHN_MASK;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
> > + val |= SPRD_DMA_GLB_DEST_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> > break;
> >
> > @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > SPRD_DMA_GLB_DEST_CHN_MASK;
> > val |= SPRD_DMA_GLB_2STAGE_EN;
> > + if (schan->int_type != SPRD_DMA_NO_INT)
> > + val |= SPRD_DMA_GLB_DEST_INT;
> > +
> > sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> > break;
> >
> > @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
> > schan->linklist.virt_addr = 0;
> > }
> >
> > - /* Set channel mode and trigger mode for 2-stage transfer */
> > + /*
> > + * Set channel mode, interrupt mode and trigger mode for 2-stage
> > + * transfer.
> > + */
> > schan->chn_mode =
> > (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
> > schan->trg_mode =
> > (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> > + schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
> >
> > sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> > if (!sdesc)
> > --
> > 1.7.9.5
>
> --
> ~Vinod
^ permalink raw reply
* Re: [PATCH 1/7] dmaengine: sprd: Fix the possible crash when getting engine status
From: Vinod Koul @ 2019-04-29 12:02 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
In-Reply-To: <CAMz4kuK0jurdNX3Z+zH5_ErR-64k2-Nuw_1T+X4OjbjURDSnUA@mail.gmail.com>
On 29-04-19, 19:49, Baolin Wang wrote:
> Hi Vinod,
>
> On Mon, 29 Apr 2019 at 19:36, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> > > has been submitted, that will crash the kernel when getting the engine status.
> >
> > No that is wrong, status is for descriptor and not engine!
>
> Sure, will fix the commit message.
>
> >
> > > In this case, since the descriptor has been submitted, which means the pointer
> > > 'schan->cur_desc' will point to the current descriptor, then we can use
> > > 'schan->cur_desc' to get the engine status to avoid this issue.
> >
> > Nope, since the descriptor is completed, you return with residue as 0
> > and DMA_COMPLETE status!
>
> No, the descriptor is not completed now. If it is completed, we will
> return 0 with DMA_COMPLETE status. But now the descriptor is on
> progress, we should get the descriptor to return current residue.
> Sorry for confusing description.
OKay will wait for updated description to understand the fix
>
> >
> > >
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > > drivers/dma/sprd-dma.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 48431e2..e29342a 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> > > else
> > > pos = 0;
> > > } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> > > - struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> > > + struct sprd_dma_desc *sdesc = schan->cur_desc;
> > >
> > > if (sdesc->dir == DMA_DEV_TO_MEM)
> > > pos = sprd_dma_get_dst_addr(schan);
> > > --
> > > 1.7.9.5
> >
> > --
> > ~Vinod
>
>
>
> --
> Baolin Wang
> Best Regards
--
~Vinod
^ permalink raw reply
* [1/7] dmaengine: sprd: Fix the possible crash when getting engine status
From: Vinod Koul @ 2019-04-29 12:02 UTC (permalink / raw)
To: Baolin Wang
Cc: Dan Williams, eric.long, Orson Zhai, Chunyan Zhang, Mark Brown,
dmaengine, LKML
On 29-04-19, 19:49, Baolin Wang wrote:
> Hi Vinod,
>
> On Mon, 29 Apr 2019 at 19:36, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > We will get a NULL virtual descriptor by vchan_find_desc() when the descriptor
> > > has been submitted, that will crash the kernel when getting the engine status.
> >
> > No that is wrong, status is for descriptor and not engine!
>
> Sure, will fix the commit message.
>
> >
> > > In this case, since the descriptor has been submitted, which means the pointer
> > > 'schan->cur_desc' will point to the current descriptor, then we can use
> > > 'schan->cur_desc' to get the engine status to avoid this issue.
> >
> > Nope, since the descriptor is completed, you return with residue as 0
> > and DMA_COMPLETE status!
>
> No, the descriptor is not completed now. If it is completed, we will
> return 0 with DMA_COMPLETE status. But now the descriptor is on
> progress, we should get the descriptor to return current residue.
> Sorry for confusing description.
OKay will wait for updated description to understand the fix
>
> >
> > >
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > > drivers/dma/sprd-dma.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 48431e2..e29342a 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -625,7 +625,7 @@ static enum dma_status sprd_dma_tx_status(struct dma_chan *chan,
> > > else
> > > pos = 0;
> > > } else if (schan->cur_desc && schan->cur_desc->vd.tx.cookie == cookie) {
> > > - struct sprd_dma_desc *sdesc = to_sprd_dma_desc(vd);
> > > + struct sprd_dma_desc *sdesc = schan->cur_desc;
> > >
> > > if (sdesc->dir == DMA_DEV_TO_MEM)
> > > pos = sprd_dma_get_dst_addr(schan);
> > > --
> > > 1.7.9.5
> >
> > --
> > ~Vinod
>
>
>
> --
> Baolin Wang
> Best Regards
^ permalink raw reply
* Re: [PATCH 7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Vinod Koul @ 2019-04-29 12:01 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
In-Reply-To: <07c070b4397296a4500d04abe16dfd8a71a2f211.1555330115.git.baolin.wang@linaro.org>
On 15-04-19, 20:15, Baolin Wang wrote:
> For 2-stage transfer, some users like Audio still need transaction interrupt
> to notify when the 2-stage transfer is completed. Thus we should enable
> 2-stage transfer interrupt to support this feature.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index cc9c24d..4c18f44 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -62,6 +62,8 @@
> /* SPRD_DMA_GLB_2STAGE_GRP register definition */
> #define SPRD_DMA_GLB_2STAGE_EN BIT(24)
> #define SPRD_DMA_GLB_CHN_INT_MASK GENMASK(23, 20)
> +#define SPRD_DMA_GLB_DEST_INT BIT(22)
> +#define SPRD_DMA_GLB_SRC_INT BIT(20)
> #define SPRD_DMA_GLB_LIST_DONE_TRG BIT(19)
> #define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
> #define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
> @@ -135,6 +137,7 @@
> /* define DMA channel mode & trigger mode mask */
> #define SPRD_DMA_CHN_MODE_MASK GENMASK(7, 0)
> #define SPRD_DMA_TRG_MODE_MASK GENMASK(7, 0)
> +#define SPRD_DMA_INT_TYPE_MASK GENMASK(7, 0)
>
> /* define the DMA transfer step type */
> #define SPRD_DMA_NONE_STEP 0
> @@ -190,6 +193,7 @@ struct sprd_dma_chn {
> u32 dev_id;
> enum sprd_dma_chn_mode chn_mode;
> enum sprd_dma_trg_mode trg_mode;
> + enum sprd_dma_int_type int_type;
> struct sprd_dma_desc *cur_desc;
> };
>
> @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
Who configure int_type?
> + val |= SPRD_DMA_GLB_SRC_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> break;
>
> @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_SRC_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> break;
>
> @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> SPRD_DMA_GLB_DEST_CHN_MASK;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_DEST_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> break;
>
> @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> SPRD_DMA_GLB_DEST_CHN_MASK;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_DEST_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> break;
>
> @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
> schan->linklist.virt_addr = 0;
> }
>
> - /* Set channel mode and trigger mode for 2-stage transfer */
> + /*
> + * Set channel mode, interrupt mode and trigger mode for 2-stage
> + * transfer.
> + */
> schan->chn_mode =
> (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
> schan->trg_mode =
> (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> + schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
>
> sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> if (!sdesc)
> --
> 1.7.9.5
--
~Vinod
^ permalink raw reply
* [7/7] dmaengine: sprd: Add interrupt support for 2-stage transfer
From: Vinod Koul @ 2019-04-29 12:01 UTC (permalink / raw)
To: Baolin Wang
Cc: dan.j.williams, eric.long, orsonzhai, zhang.lyra, broonie,
dmaengine, linux-kernel
On 15-04-19, 20:15, Baolin Wang wrote:
> For 2-stage transfer, some users like Audio still need transaction interrupt
> to notify when the 2-stage transfer is completed. Thus we should enable
> 2-stage transfer interrupt to support this feature.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> drivers/dma/sprd-dma.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index cc9c24d..4c18f44 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -62,6 +62,8 @@
> /* SPRD_DMA_GLB_2STAGE_GRP register definition */
> #define SPRD_DMA_GLB_2STAGE_EN BIT(24)
> #define SPRD_DMA_GLB_CHN_INT_MASK GENMASK(23, 20)
> +#define SPRD_DMA_GLB_DEST_INT BIT(22)
> +#define SPRD_DMA_GLB_SRC_INT BIT(20)
> #define SPRD_DMA_GLB_LIST_DONE_TRG BIT(19)
> #define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
> #define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
> @@ -135,6 +137,7 @@
> /* define DMA channel mode & trigger mode mask */
> #define SPRD_DMA_CHN_MODE_MASK GENMASK(7, 0)
> #define SPRD_DMA_TRG_MODE_MASK GENMASK(7, 0)
> +#define SPRD_DMA_INT_TYPE_MASK GENMASK(7, 0)
>
> /* define the DMA transfer step type */
> #define SPRD_DMA_NONE_STEP 0
> @@ -190,6 +193,7 @@ struct sprd_dma_chn {
> u32 dev_id;
> enum sprd_dma_chn_mode chn_mode;
> enum sprd_dma_trg_mode trg_mode;
> + enum sprd_dma_int_type int_type;
> struct sprd_dma_desc *cur_desc;
> };
>
> @@ -429,6 +433,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
Who configure int_type?
> + val |= SPRD_DMA_GLB_SRC_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> break;
>
> @@ -436,6 +443,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_SRC_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> break;
>
> @@ -443,6 +453,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> SPRD_DMA_GLB_DEST_CHN_MASK;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_DEST_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> break;
>
> @@ -450,6 +463,9 @@ static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> SPRD_DMA_GLB_DEST_CHN_MASK;
> val |= SPRD_DMA_GLB_2STAGE_EN;
> + if (schan->int_type != SPRD_DMA_NO_INT)
> + val |= SPRD_DMA_GLB_DEST_INT;
> +
> sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> break;
>
> @@ -911,11 +927,15 @@ static int sprd_dma_fill_linklist_desc(struct dma_chan *chan,
> schan->linklist.virt_addr = 0;
> }
>
> - /* Set channel mode and trigger mode for 2-stage transfer */
> + /*
> + * Set channel mode, interrupt mode and trigger mode for 2-stage
> + * transfer.
> + */
> schan->chn_mode =
> (flags >> SPRD_DMA_CHN_MODE_SHIFT) & SPRD_DMA_CHN_MODE_MASK;
> schan->trg_mode =
> (flags >> SPRD_DMA_TRG_MODE_SHIFT) & SPRD_DMA_TRG_MODE_MASK;
> + schan->int_type = flags & SPRD_DMA_INT_TYPE_MASK;
>
> sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT);
> if (!sdesc)
> --
> 1.7.9.5
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox