[PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Vinod Koul]

On Mon, May 04, 2015 at 04:58:25PM +0200, Ludovic Desroches wrote:
> On Mon, May 04, 2015 at 04:06:44PM +0530, Vinod Koul wrote:
> > On Mon, May 04, 2015 at 11:12:41AM +0200, Ludovic Desroches wrote:
> > > On Mon, May 04, 2015 at 01:55:36PM +0530, Vinod Koul wrote:
> > > > On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> > > > > Hi Vinod,
> > > > > 
> > > > > On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > > > > > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > > > > > When doing the slave configuration, an error is returned if the maxburst
> > > > > > > value is not supported. The bug comes from the fact that we always check
> > > > > > > the maxburst for both directions but in the case of an unidirectional
> > > > > > > channel, only one is set.
> > > > > > While setting the slave configuration we are not tied to a channel
> > > > > > direction, the direction is passed usin prep_ method. So from that
> > > > > > prespective a channle can be used for tx and rx with same slave config set.
> > > > > > 
> > > > > > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > > > > > would have been fine but here we are checking when the slave config is set
> > > > > > so this is not right. You should check maxburst at runtime then...
> > > > > > 
> > > > > 
> > > > > I don't understand why we should wait before checking the
> > > > > configuration... Some channels are unidirectionnal so implicitly we know
> > > > > the direction at configuration time because the device will fill only a
> > > > > part of the dma_slave_config structure. For example, the atmel usart
> > > > > requests a tx and a rx channels. When configuring the tx channel, only
> > > > > the dst_ fields of the dma_slave_config structure are filled. Is it a
> > > > > bad behavior?
> > > > > 
> > > > > The change introduced by this patch doesn't really care about the
> > > > > direction, it only tells that if the device only fills src_ fields then
> > > > > I don't have to check fields not configured.
> > > > Well that is because we started with the assumption that channels are
> > > > uni-direction and we know that. From client side we shouldn't care
> > > > how channel looks like and which dma controller we are talking. The point is
> > > > to make clients unaware and use the dmaengine API
> > > 
> > > I am sorry but I don't understand what is wrong with your vision and
> > > this patch.
> > > 
> > > I think it is the case you describe, the client doesn't care how the
> > > channel looks like, it wants a channel for doing transmission so it
> > > fills only the configuration part for this purpose. In this case there
> > > is no reason the dma controller returns an error by checking unset
> > > values.
> > As i said earlier, if the checks were in prepare call then they would be
> > fine, but they are in slave_dma_config API. You dont know the direction at
> > this point so checking here is not good. You should copy the parameters here
> > as most do and move the checks to prepare_ calls where you know the
> > direction for argument of prepare and return accordingly.
> > 
> > > 
> > > It's true I can check the configuration requested by the client at
> > > runtime. But why waiting so long? If there is an issue, it's better to
> > > return the error as soon as possible. On client side, even at this
> > > moment it is difficult to manage an error. For example, my dma
> > > controller doesn't support the max burst requested, how the client will
> > > know what is the maxburst size it can ask?
> > Because you dont have the complete information. And failing prepare at that
> > time makese sense because parameters have not be set properly!
> > 
> 
> Sorry but I still don't understand why I don't have the complete
> information. The only thing missing is the direction and I don't care
> about it.
> 
> I was updating my patch but I realize I have no benefit to do it in
> prep. Worse if I simply copy the slave configuration and check it in prep, I'll
> check the maxburst value several times instead of one and it will always be
> the same...
ah yes but then you can use the channel for either direction without setting
slave_dma_config...

-- 
~Vinod