On Mon, Apr 15, 2013 at 1:59 PM, Lee Jones <lee.jo...@linaro.org> wrote: > On Mon, 15 Apr 2013, Rabin Vincent wrote: >> 2013/4/15 Lee Jones <lee.jo...@linaro.org>: >> > On Fri, 12 Apr 2013, Rabin Vincent wrote: >> >> 2013/4/9 Lee Jones <lee.jo...@linaro.org>: >> >> > 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. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
