| Message ID | 20170513221327.10114-2-alexander.sverdlin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, May 14, 2017 at 12:13:26AM +0200, Alexander Sverdlin wrote: > The current buffer is being reset to zero on device_free_chan_resources() > but not on device_terminate_all(). It could happen that HW is restarted and > expects BASE0 to be used, but the driver is not synchronized and will start > from BASE1. Therefore, it's better to reset the buffer explicitly in > m2p_hw_setup(). While here, check that HW really starts from BASE0 and > warn otherwise. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/dma/ep93xx_dma.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c > index d37e8dda8079..a3d946d2a8e1 100644 > --- a/drivers/dma/ep93xx_dma.c > +++ b/drivers/dma/ep93xx_dma.c > @@ -45,6 +45,7 @@ > > #define M2P_PPALLOC 0x0008 > #define M2P_STATUS 0x000c > +#define M2P_STATUS_NEXTBUFFER BIT(6) > > #define M2P_MAXCNT0 0x0020 > #define M2P_BASE0 0x0024 > @@ -312,6 +313,13 @@ static void m2p_set_control(struct ep93xx_dma_chan *edmac, u32 control) > readl(edmac->regs + M2P_CONTROL); > } > > +static inline u32 m2p_channel_nextbuf(struct ep93xx_dma_chan *edmac) > +{ > + if (readl(edmac->regs + M2P_STATUS) & M2P_STATUS_NEXTBUFFER) > + return 1; > + return 0; > +} > + > static int m2p_hw_setup(struct ep93xx_dma_chan *edmac) > { > struct ep93xx_dma_data *data = edmac->chan.private; > @@ -323,6 +331,10 @@ static int m2p_hw_setup(struct ep93xx_dma_chan *edmac) > | M2P_CONTROL_ENABLE; > m2p_set_control(edmac, control); > > + if (m2p_channel_nextbuf(edmac) != 0) > + dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n"); But then you are actually not restarting from BASE1 as you reset, so the warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0 but found 1, so resetting" > + edmac->buffer = 0; > + > return 0; > } > > -- > 2.12.2 >
Hello Vinod, On 14/05/17 15:14, Vinod Koul wrote: >> @@ -323,6 +331,10 @@ static int m2p_hw_setup(struct ep93xx_dma_chan *edmac) >> | M2P_CONTROL_ENABLE; >> m2p_set_control(edmac, control); >> >> + if (m2p_channel_nextbuf(edmac) != 0) >> + dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n"); > But then you are actually not restarting from BASE1 as you reset, so the > warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0 > but found 1, so resetting" This is, maybe not so obvious, a warning, which should never fire. It just proves, that I understand the HW correctly. As long as nobody sees the warning, it's the case. And it proves that HW behaves according to user manual. We can remove this warning completely. Once again, the story is following: - m2p_hw_setup() is always called after channel reset - after reset channel expects to start from buffer 0 - therefore we must always start from buffer 0 <= this was not the case before - we can read the expected "next buffer" from the controller => hence the warning maybe if there will be a bug somewhere and the channel will not be reset before next transfer, it will be triggered Regards, Alexander. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Vinod, On 14/05/17 15:33, Alexander Sverdlin wrote: >>> + if (m2p_channel_nextbuf(edmac) != 0) >>> + dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n"); >> But then you are actually not restarting from BASE1 as you reset, so the >> warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0 >> but found 1, so resetting" Should it be something like "M2P: Channel has not been reset properly!\n"? > This is, maybe not so obvious, a warning, which should never fire. It just > proves, that I understand the HW correctly. As long as nobody sees the warning, > it's the case. And it proves that HW behaves according to user manual. > We can remove this warning completely. > > Once again, the story is following: > > - m2p_hw_setup() is always called after channel reset > - after reset channel expects to start from buffer 0 > - therefore we must always start from buffer 0 <= this was not the case before > - we can read the expected "next buffer" from the controller => hence the warning > maybe if there will be a bug somewhere and the channel will not be reset before > next transfer, it will be triggered -- Alexander. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 14, 2017 at 03:36:56PM +0200, Alexander Sverdlin wrote: > Hello Vinod, > > On 14/05/17 15:33, Alexander Sverdlin wrote: > >>> + if (m2p_channel_nextbuf(edmac) != 0) > >>> + dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n"); > >> But then you are actually not restarting from BASE1 as you reset, so the > >> warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0 > >> but found 1, so resetting" > > Should it be something like "M2P: Channel has not been reset properly!\n"? Better and with a word that you recovered and did reset of the state > > This is, maybe not so obvious, a warning, which should never fire. It just > > proves, that I understand the HW correctly. As long as nobody sees the warning, > > it's the case. And it proves that HW behaves according to user manual. > > We can remove this warning completely. > > > > Once again, the story is following: > > > > - m2p_hw_setup() is always called after channel reset > > - after reset channel expects to start from buffer 0 > > - therefore we must always start from buffer 0 <= this was not the case before > > - we can read the expected "next buffer" from the controller => hence the warning > > maybe if there will be a bug somewhere and the channel will not be reset before > > next transfer, it will be triggered > > -- > Alexander.
Hi! On 14/05/17 17:17, Vinod Koul wrote: > On Sun, May 14, 2017 at 03:36:56PM +0200, Alexander Sverdlin wrote: >> Hello Vinod, >> >> On 14/05/17 15:33, Alexander Sverdlin wrote: >>>>> + if (m2p_channel_nextbuf(edmac) != 0) >>>>> + dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n"); >>>> But then you are actually not restarting from BASE1 as you reset, so the >>>> warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0 >>>> but found 1, so resetting" >> Should it be something like "M2P: Channel has not been reset properly!\n"? > Better and with a word that you recovered and did reset of the state No, no, I do not touch HW at all, I make the driver state consistent. So the condition for warning is not expected at all. It could be even BUG_ON(); Only in case someone brakes the driver or changes the dmaengine core. The problem was: we are maintaining the shadow of the controller state in the driver. This "->buffer" member. It was not properly reset when the controller was. Alexander. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c index d37e8dda8079..a3d946d2a8e1 100644 --- a/drivers/dma/ep93xx_dma.c +++ b/drivers/dma/ep93xx_dma.c @@ -45,6 +45,7 @@ #define M2P_PPALLOC 0x0008 #define M2P_STATUS 0x000c +#define M2P_STATUS_NEXTBUFFER BIT(6) #define M2P_MAXCNT0 0x0020 #define M2P_BASE0 0x0024 @@ -312,6 +313,13 @@ static void m2p_set_control(struct ep93xx_dma_chan *edmac, u32 control) readl(edmac->regs + M2P_CONTROL); } +static inline u32 m2p_channel_nextbuf(struct ep93xx_dma_chan *edmac) +{ + if (readl(edmac->regs + M2P_STATUS) & M2P_STATUS_NEXTBUFFER) + return 1; + return 0; +} + static int m2p_hw_setup(struct ep93xx_dma_chan *edmac) { struct ep93xx_dma_data *data = edmac->chan.private; @@ -323,6 +331,10 @@ static int m2p_hw_setup(struct ep93xx_dma_chan *edmac) | M2P_CONTROL_ENABLE; m2p_set_control(edmac, control); + if (m2p_channel_nextbuf(edmac) != 0) + dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n"); + edmac->buffer = 0; + return 0; }
The current buffer is being reset to zero on device_free_chan_resources() but not on device_terminate_all(). It could happen that HW is restarted and expects BASE0 to be used, but the driver is not synchronized and will start from BASE1. Therefore, it's better to reset the buffer explicitly in m2p_hw_setup(). While here, check that HW really starts from BASE0 and warn otherwise. Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> Cc: stable@vger.kernel.org --- drivers/dma/ep93xx_dma.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)