Re: [PATCH 3/8] dmaengine: ste_dma40: Actually write the runtimeconfiguration to registers

[Lee Jones]

On Mon, 15 Apr 2013, Linus Walleij wrote:

> On Mon, Apr 15, 2013 at 1:59 PM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > On Mon, 15 Apr 2013, Rabin Vincent wrote:
> >> 2013/4/15 Lee Jones <lee.jones@xxxxxxxxxx>:
> >> > On Fri, 12 Apr 2013, Rabin Vincent wrote:
> >> >> 2013/4/9 Lee Jones <lee.jones@xxxxxxxxxx>:
> >> >> > Someone has spent a fair amount of effort writing a runtime configuration
> >> >> > changing algorithm for DMA clients. However, the config appears to never
> >> >> > actually make it to hardware. In order for the configuration to take hold
> >> >> > we need to issue a d40_config_write(), as this is the routine which writes
> >> >> > it into the hardware's registers.
> >> >>
> >> >> No, it's not.  This function is only for initial configuration which
> >> >> should only be written when the channel is allocated.  In fact, by
> >> >> calling it here in runtime_config, you are introducing a serious bug:
> >> >> other logical channels on the same physical channel will stop because of
> >> >> the SSLNK/SDLNK of the physical channel being zeroed.
> >> >>
> >> >> The runtime config already makes it the hardware in the existing code,
> >> >> via d40_*_cfg().
> >> >
> >> > Sorry Rabin, but the only place I can see the config being written is
> >> > in d40_config_write().
> >> >
> >> > Can you paste the line of code in d40_*_cfg() which actually writes
> >> > the config to hardware please? I don't see it.
> >>
> >> It's not that simple.  There are some pointers passed to d40_*_cfg() and
> >> that function writes the configuration to the variables those pointers
> >> point to (d40c->log_def.lcsp1, d40c->src_def_cfg, etc.).  Please read
> >> the code to see how those variables end up being used later when the
> >> LLIs are prepared for the HW.
> >
> > I have read the code, which is why I know that the config only gets
> > written in d40_config_write(). :)
> >
> > So the configuration which gets set in the runtime_config routine
> > doesn't ever make it to hardware - hence this patch.
> >
> > Unless I'm missing something?
> 
> The runtime config sets up the config for all *subsequent* jobs.
> struct d40_chan contains e.g. runtime_addr, runtime_direction
> etc to store up the stuff being configured. This is done for
> all the other config as well using d40_log_cfg() or
> d40_phy_cfg(), and the result is cached in the channel
> struct, here called d40c.
> 
> Next, when preparing jobs, the DMA40 LLI engine will fill in
> job descriptors using e.g. d40_get_dev_addr()
> and pick settings from this cached runtime config.
> 
> When the subsequent jobs trigger, it allocates channel resources
> by calling d40_alloc_chan_resources(). The config is written to the
> hardware by calling d40_config_write().
> 
> So the basic misunderstanding here is that you think
> the config shall take effect immediately, that is not the
> idea. The configuration will be cached in the channel
> struct, then it will take effect when the next job that is
> queued up allocates its resources to commence.
> 
> Maybe you haven't grasped the asynchronous nature
> of the DMAengine? It's a bit like multithreading. You
> queue up things, then they commence at a later time.
> Not immediately.
> 
> Example: the SPI driver for PL022 may talk to a
> 8, 16 or 32bit capable device. Depending on which device
> it wants to queue a job for, it need to be configured differently,
> like for another width.
> 
> At the time you are queueing a new job, another job may
> already be in flight. If you issue d40_config_write() at this
> point, you will cause undesired effects to the job that may
> already be running. Such as reconfiguring the width
> of the channel.
> 
> You may not observe that bug because there is no
> job in flight, or because the actual configuration is identical
> for two consecutive jobs, but if you're really stressing something,
> queuing jobs while others are already in flight, you will
> see it immediately.
> 
> Therefore, this patch should be dropped.

Ah right, thanks for your explanation.

I have also just finished speaking to Rabin privately, who painted a
good picture of how the configuration side of this driver should
work.

I have dropped this patch and will shortly be replacing it with
something else.

Thanks again to both of you.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/