| Message ID | 1448961299-15161-4-git-send-email-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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.
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
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.
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
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?
On Thu, Dec 10, 2015 at 03:18:33PM +0200, Stanimir Varbanov wrote: <snip> > >>> 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? Yes that's fine with me.
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 */
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(-)