Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280674 > unrolled thread

[PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size

Started byStanimir Varbanov <stanimir.varbanov@linaro.org>
First post2015-12-01 10:20 +0100
Last post2015-12-10 14:20 +0100
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2015-12-01 10:20 +0100
    Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Arnd Bergmann <arnd@arndb.de> - 2015-12-01 11:30 +0100
      Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Andy Gross <agross@codeaurora.org> - 2015-12-01 18:30 +0100
        Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Arnd Bergmann <arnd@arndb.de> - 2015-12-01 21:30 +0100
    Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Andy Gross <agross@codeaurora.org> - 2015-12-01 18:30 +0100
      Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2015-12-02 17:50 +0100
        Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Andy Gross <agross@codeaurora.org> - 2015-12-02 18:30 +0100
          Re: [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size Stanimir Varbanov <stanimir.varbanov@linaro.org> - 2015-12-10 14:20 +0100

#1280674 — [PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size

FromStanimir Varbanov <stanimir.varbanov@linaro.org>
Date2015-12-01 10:20 +0100
Subject[PATCH 3/4] dmaengine: qcom_bam_dma: use correct pipe FIFO size
Message-ID<qAPgm-7I5-21@gated-at.bofh.it>
The pipe fifo size register must instruct the bam hw
how many hw descriptors can be pushed to fifo. Currently
we isntruct the hw with 32KBytes but wrap the tail in
bam_start_dma in BAM_P_EVNT_REG on 4095 i.e. 32760. This
leads to stalled transactions when the tail wraps.

Fix this by use the correct fifo size in BAM_P_FIFO_SIZES
register i.e. 32K - 8.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/dma/qcom_bam_dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
index 0f06f3b7a72b..6d290de9ab2b 100644
--- a/drivers/dma/qcom_bam_dma.c
+++ b/drivers/dma/qcom_bam_dma.c
@@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
 	 */
 	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
 			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
-	writel_relaxed(BAM_DESC_FIFO_SIZE,
+	writel_relaxed(BAM_MAX_DATA_SIZE,
 			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
 
 	/* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1280734

FromArnd Bergmann <arnd@arndb.de>
Date2015-12-01 11:30 +0100
Message-ID<qAQm5-8lS-9@gated-at.bofh.it>
In reply to#1280674
On Tuesday 01 December 2015 11:14:58 Stanimir Varbanov wrote:
> 
> diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
> index 0f06f3b7a72b..6d290de9ab2b 100644
> --- a/drivers/dma/qcom_bam_dma.c
> +++ b/drivers/dma/qcom_bam_dma.c
> @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
>          */
>         writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
>                         bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
> -       writel_relaxed(BAM_DESC_FIFO_SIZE,
> +       writel_relaxed(BAM_MAX_DATA_SIZE,
>                         bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
>  
>         /* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */

I'm looking at that now and fail to see why these have to use writel_relaxed().

Could you add a patch to use readl/writel by default?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281064

FromAndy Gross <agross@codeaurora.org>
Date2015-12-01 18:30 +0100
Message-ID<qAWUx-49J-5@gated-at.bofh.it>
In reply to#1280734
On Tue, Dec 01, 2015 at 11:28:32AM +0100, Arnd Bergmann wrote:
> On Tuesday 01 December 2015 11:14:58 Stanimir Varbanov wrote:
> > 
> > diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
> > index 0f06f3b7a72b..6d290de9ab2b 100644
> > --- a/drivers/dma/qcom_bam_dma.c
> > +++ b/drivers/dma/qcom_bam_dma.c
> > @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
> >          */
> >         writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
> >                         bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
> > -       writel_relaxed(BAM_DESC_FIFO_SIZE,
> > +       writel_relaxed(BAM_MAX_DATA_SIZE,
> >                         bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
> >  
> >         /* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */
> 
> I'm looking at that now and fail to see why these have to use writel_relaxed().

At some point I believe I got a comment about using (readl/writel)_relaxed
instead of readl/writel.  So I used these instead.  Has the wind direction
changed?  =)

Using the readl/writel is nice w.r.t. having the implicit barriers, especially
with the funky 1K boundary on reordering of operations that can occur on Kraits.
This can hit you on accesses even within the same IP block.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281186

FromArnd Bergmann <arnd@arndb.de>
Date2015-12-01 21:30 +0100
Message-ID<qAZIK-5Y4-21@gated-at.bofh.it>
In reply to#1281064
On Tuesday 01 December 2015 11:25:35 Andy Gross wrote:
> On Tue, Dec 01, 2015 at 11:28:32AM +0100, Arnd Bergmann wrote:
> > On Tuesday 01 December 2015 11:14:58 Stanimir Varbanov wrote:
> > > 
> > > diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
> > > index 0f06f3b7a72b..6d290de9ab2b 100644
> > > --- a/drivers/dma/qcom_bam_dma.c
> > > +++ b/drivers/dma/qcom_bam_dma.c
> > > @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
> > >          */
> > >         writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
> > >                         bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
> > > -       writel_relaxed(BAM_DESC_FIFO_SIZE,
> > > +       writel_relaxed(BAM_MAX_DATA_SIZE,
> > >                         bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
> > >  
> > >         /* enable the per pipe interrupts, enable EOT, ERR, and INT irqs */
> > 
> > I'm looking at that now and fail to see why these have to use writel_relaxed().
> 
> At some point I believe I got a comment about using (readl/writel)_relaxed
> instead of readl/writel.  So I used these instead.  Has the wind direction
> changed?  =)

Yes.

> Using the readl/writel is nice w.r.t. having the implicit barriers, especially
> with the funky 1K boundary on reordering of operations that can occur on Kraits.
> This can hit you on accesses even within the same IP block.

We had a couple of bugs that we should not have had when drivers were mindlessly
converted, so generally speaking at least I try to get people to only use
the relaxed functions for the hot path when they can show an advantage as well
as the fact that it's safe to use.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281075

FromAndy Gross <agross@codeaurora.org>
Date2015-12-01 18:30 +0100
Message-ID<qAWUy-49J-41@gated-at.bofh.it>
In reply to#1280674
On Tue, Dec 01, 2015 at 11:14:58AM +0200, Stanimir Varbanov wrote:
> The pipe fifo size register must instruct the bam hw
> how many hw descriptors can be pushed to fifo. Currently
> we isntruct the hw with 32KBytes but wrap the tail in
> bam_start_dma in BAM_P_EVNT_REG on 4095 i.e. 32760. This
> leads to stalled transactions when the tail wraps.
> 
> Fix this by use the correct fifo size in BAM_P_FIFO_SIZES
> register i.e. 32K - 8.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/dma/qcom_bam_dma.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
> index 0f06f3b7a72b..6d290de9ab2b 100644
> --- a/drivers/dma/qcom_bam_dma.c
> +++ b/drivers/dma/qcom_bam_dma.c
> @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
>  	 */
>  	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
>  			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
> -	writel_relaxed(BAM_DESC_FIFO_SIZE,
> +	writel_relaxed(BAM_MAX_DATA_SIZE,
>  			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));

This is just using the #define.  That is ok, but if you use this instead of the
BAM_P_FIFO_SIZES then you need to fix your comment.  Or actually use the
register value.... otherwise looks fine.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282004

FromStanimir Varbanov <stanimir.varbanov@linaro.org>
Date2015-12-02 17:50 +0100
Message-ID<qBiLo-1hG-13@gated-at.bofh.it>
In reply to#1281075
On 12/01/2015 07:23 PM, Andy Gross wrote:
> On Tue, Dec 01, 2015 at 11:14:58AM +0200, Stanimir Varbanov wrote:
>> The pipe fifo size register must instruct the bam hw
>> how many hw descriptors can be pushed to fifo. Currently
>> we isntruct the hw with 32KBytes but wrap the tail in
>> bam_start_dma in BAM_P_EVNT_REG on 4095 i.e. 32760. This
>> leads to stalled transactions when the tail wraps.
>>
>> Fix this by use the correct fifo size in BAM_P_FIFO_SIZES
>> register i.e. 32K - 8.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/dma/qcom_bam_dma.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
>> index 0f06f3b7a72b..6d290de9ab2b 100644
>> --- a/drivers/dma/qcom_bam_dma.c
>> +++ b/drivers/dma/qcom_bam_dma.c
>> @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
>>  	 */
>>  	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
>>  			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
>> -	writel_relaxed(BAM_DESC_FIFO_SIZE,
>> +	writel_relaxed(BAM_MAX_DATA_SIZE,
>>  			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
> 
> This is just using the #define.  That is ok, but if you use this instead of the
> BAM_P_FIFO_SIZES then you need to fix your comment.  Or actually use the
> register value.... otherwise looks fine.

I did not follow your comment, but the intension of the patch is to set
the proper FIFO size in BAM_P_FIFO_SIZES register, i.e. 32K - 8.

-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282103

FromAndy Gross <agross@codeaurora.org>
Date2015-12-02 18:30 +0100
Message-ID<qBjo6-1O1-27@gated-at.bofh.it>
In reply to#1282004
On Wed, Dec 02, 2015 at 06:44:11PM +0200, Stanimir Varbanov wrote:
> On 12/01/2015 07:23 PM, Andy Gross wrote:
> > On Tue, Dec 01, 2015 at 11:14:58AM +0200, Stanimir Varbanov wrote:
> >> The pipe fifo size register must instruct the bam hw
> >> how many hw descriptors can be pushed to fifo. Currently
> >> we isntruct the hw with 32KBytes but wrap the tail in
> >> bam_start_dma in BAM_P_EVNT_REG on 4095 i.e. 32760. This
> >> leads to stalled transactions when the tail wraps.
> >>
> >> Fix this by use the correct fifo size in BAM_P_FIFO_SIZES
> >> register i.e. 32K - 8.
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  drivers/dma/qcom_bam_dma.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
> >> index 0f06f3b7a72b..6d290de9ab2b 100644
> >> --- a/drivers/dma/qcom_bam_dma.c
> >> +++ b/drivers/dma/qcom_bam_dma.c
> >> @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
> >>  	 */
> >>  	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
> >>  			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
> >> -	writel_relaxed(BAM_DESC_FIFO_SIZE,
> >> +	writel_relaxed(BAM_MAX_DATA_SIZE,
> >>  			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
> > 
> > This is just using the #define.  That is ok, but if you use this instead of the
> > BAM_P_FIFO_SIZES then you need to fix your comment.  Or actually use the
> > register value.... otherwise looks fine.
> 
> I did not follow your comment, but the intension of the patch is to set
> the proper FIFO size in BAM_P_FIFO_SIZES register, i.e. 32K - 8.

Sorry, I mixed up the usage and was thinking there was something you read out
that told you the size.  That's not how it works, unfortunately.  The
MAX_DATA_SIZE is fine, but the name is a little misleading.  Perhaps just
BAM_FIFO_SIZE?


Regards,

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1288509

FromStanimir Varbanov <stanimir.varbanov@linaro.org>
Date2015-12-10 14:20 +0100
Message-ID<qE9iz-6It-27@gated-at.bofh.it>
In reply to#1282103
On 12/02/2015 07:22 PM, Andy Gross wrote:
> On Wed, Dec 02, 2015 at 06:44:11PM +0200, Stanimir Varbanov wrote:
>> On 12/01/2015 07:23 PM, Andy Gross wrote:
>>> On Tue, Dec 01, 2015 at 11:14:58AM +0200, Stanimir Varbanov wrote:
>>>> The pipe fifo size register must instruct the bam hw
>>>> how many hw descriptors can be pushed to fifo. Currently
>>>> we isntruct the hw with 32KBytes but wrap the tail in
>>>> bam_start_dma in BAM_P_EVNT_REG on 4095 i.e. 32760. This
>>>> leads to stalled transactions when the tail wraps.
>>>>
>>>> Fix this by use the correct fifo size in BAM_P_FIFO_SIZES
>>>> register i.e. 32K - 8.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  drivers/dma/qcom_bam_dma.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/dma/qcom_bam_dma.c b/drivers/dma/qcom_bam_dma.c
>>>> index 0f06f3b7a72b..6d290de9ab2b 100644
>>>> --- a/drivers/dma/qcom_bam_dma.c
>>>> +++ b/drivers/dma/qcom_bam_dma.c
>>>> @@ -458,7 +458,7 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
>>>>  	 */
>>>>  	writel_relaxed(ALIGN(bchan->fifo_phys, sizeof(struct bam_desc_hw)),
>>>>  			bam_addr(bdev, bchan->id, BAM_P_DESC_FIFO_ADDR));
>>>> -	writel_relaxed(BAM_DESC_FIFO_SIZE,
>>>> +	writel_relaxed(BAM_MAX_DATA_SIZE,
>>>>  			bam_addr(bdev, bchan->id, BAM_P_FIFO_SIZES));
>>>
>>> This is just using the #define.  That is ok, but if you use this instead of the
>>> BAM_P_FIFO_SIZES then you need to fix your comment.  Or actually use the
>>> register value.... otherwise looks fine.
>>
>> I did not follow your comment, but the intension of the patch is to set
>> the proper FIFO size in BAM_P_FIFO_SIZES register, i.e. 32K - 8.
> 
> Sorry, I mixed up the usage and was thinking there was something you read out
> that told you the size.  That's not how it works, unfortunately.  The
> MAX_DATA_SIZE is fine, but the name is a little misleading.  Perhaps just
> BAM_FIFO_SIZE?

OK I can rename BAM_MAX_DATA_SIZE to BAM_FIFO_SIZE, and use it when
setting BAM_P_FIFO_SIZES register. Is that fine to you?


-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web