On Sat, May 18, 2019 at 2:34 AM Alex Elder <el...@linaro.org> wrote: > On 5/15/19 7:35 AM, Alex Elder wrote: > > On 5/15/19 3:16 AM, Arnd Bergmann wrote: > >> > >> This looks rather strange. I think I looked at it before and you explained > >> it, but I have since forgotten what you do it for, so I assume everyone > >> else > >> that tries to understand this will have problems too. > > > > This is a bug. I think I misunderstood why you were > > puzzled before. Now I get it. I need to save that > > DMA address and not free it at the end of the function > > (except on error). > > OK, now I'm going to correct myself. I hope I don't make > any mistakes here because things are confused enough... > > Part of what I described previously is still true, namely > there are tables that need to be initialized (i.e., the > IPA needs to be told where they reside), and there is a > separate step is available to zero the content of the tables. > > But there really is no need for the AP to hang onto this > DMA memory after this immediate command has been issued. > I will add comments in the code to make it less surprising. > > But here's a summary of why. > > I think there are two things at play that make it confusing. > > The first thing is that these "header tables" are actually > located in a region of shared memory ("smem") that is local > to the IPA (not the AP). The the IPA_CMD_HDR_INIT_LOCAL > immediate command is meant to: > 1) define the header table location in IPA local memory > 2) define the header table size > 3) provide a buffer used to fill the table with its initial > contents > > The location and size are encoded in the flags field > of the payload (offset and size). > > The initial contents are filled via DMA from a buffer > in main memory, whose DMA address is supplied in the > hdr_table_addr parameter in the payload. The initial > contents we supply are all zero. So this is why we > need to allocate DMA memory. > > The second thing is that this is an instance where the > AP is responsible for performing some initialization > of resources it may not "own" thereafter. The IPA > hardware owns this table, even though the AP needs to > tell it where it sits in IPA local memory. The AP is > able to copy (using DMA) content into that table, but > doing so involves a DMA transfer. > > More advanced features of the IPA would make more use > of this header table, but those features not yet > supported so this initialization (and a subsequent, > seemingly redundant zeroing) is all we do. > > Does that make sense?
Ok, that sounds reasonable, yes. I'm not sure if dma_alloc_coherent() guarantees zero-initialization though, so if that is required, you may have to add a memset(). Arnd