From mboxrd@z Thu Jan 1 00:00:00 1970 From: Detlev Zundel Date: Fri, 13 Aug 2010 15:01:29 +0200 Subject: [U-Boot] [PATCH v2 2/3] avr32: Use uncached() macro to get an address for SDRAM init In-Reply-To: <20100813191241.7b9b9061@hskinnemoen-d830> (Haavard Skinnemoen's message of "Fri, 13 Aug 2010 19:12:41 +0700") References: <1281595974-32279-1-git-send-email-haavard.skinnemoen@atmel.com> <1281595974-32279-3-git-send-email-haavard.skinnemoen@atmel.com> <20100813105606.2e8e656d@hskinnemoen-d830> <20100813191241.7b9b9061@hskinnemoen-d830> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Haavard, > Detlev Zundel wrote: >> > Problem is that in order to make the CFI driver work on avr32, we need >> > to change the map_physmem() macro to return the physical address >> > unchanged. >> >> I see. And I presume you cannot tell the situation apart inside >> map_physmem? > > I don't think so. How do you propose we do that? I don't know, that's why I'm asking. Let's take a step back and please excuse my ignorant question - why exactly does the CFI driver need the physical address unchanged? Isn't the real constraint that the address requested by CFI is uncached? Why can't this be done by map_physmem()? >> > The map_physmem() macro currently does exactly the same thing as the >> > uncached() macro, and the unmap is a noop, but the next patch changes >> > it in order to fix the CFI driver. If the next patch is applied without >> > this patch being applied first, the SDRAM driver will do cached >> > accesses during initialization, and that may cause the initialization >> > to fail. >> >> Why not include a note to this extent into the git commit message? This >> would be a great help for other people to later understand why this >> change has been done the "backward way" that it was. > > The commit message already contains this: > > The paging system which is required to set up caching properties has not > yet been initialized when the SDRAM is initialized. So when the > map_physmem() function is converted to return the physical address > unchanged, the SDRAM initialization will break on some boards. > > which is essentially the same thing, isn't it? For me this is not the same - it does not include a word why the change which you agree "looks backward" is something that we want to do. For me something like this would give me more information: Unfortunately we cannot make "map_physmem()/unmap_physmem()" on the AVR32 platform work with the CFI driver as it works on other platforms. [I don't understand why this is the case, so your explanation would go here ;) ] To still fix the issue we deliberately replace these generic routines by AVR32 specific routines. If someone can fix this using the generic patterns, patches are welcome. I believe that good docmumentation should include such pro- and con reasoning so that code changes can be comprehended even after the fact. Cheers Detlev -- Emacs seems a more likely candidate to contain a mail system than the mail system to contain an Emacs, so this is the way it was done. -- Bernard S. Greenberg -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de