| Message ID | 20090526170653.GB24261@linux-sh.org |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 26, 2009 at 13:06, Paul Mundt wrote: > On Tue, May 26, 2009 at 12:50:51PM -0400, Mike Frysinger wrote: >> On Tue, May 26, 2009 at 12:47, Paul Mundt wrote: >> > On Tue, May 26, 2009 at 12:42:48PM -0400, Mike Frysinger wrote: >> >> > In this case you should just kill all of that crap off, and have the >> >> > platforms that use this set uclinux_ram_map up themselves, it's already >> >> > a global. Of course you can use _ebss as a default value for >> >> > uclinux_ram_map.phys and just override it in your platform. >> >> >> >> i would agree if it were a board-specific issue, but it's an arch >> >> issue, so pushing it to the boards level is wrong. ??i can however >> >> replace the addr with a global weak and add a symbol into the Blackfin >> >> arch code to override it. >> >> >> > I obviously meant architectures setting up the address, not the board >> > code, as this has nothing at all to do with boards. There are already >> > plenty of cases in setup_arch() for filling in uclinux mtd data, one more >> > isn't going to make a difference. >> > >> > I don't see anything wrong with keeping uclinux_ram_map as a global >> > however, particularly since platforms that need to special case the >> > mapping can easily do this under the existing ifdef. Adding weak symbols >> > for something like this just seems silly. >> >> the point of the weak symbol was so that the map could provide a sane >> default that works for most everyone out there without having to copy >> & paste the same code to every arch, and to make new arch porters >> worry about what needs to be done to use this very trivial map > > Did you purposely only read the parts of my email that you felt like? > Note the original quoted part that mentions using _ebss as a default and > simply overriding it in your platform. if i wanted to piss you off, i imagine that would be what i was doing. but considering my purpose is to get merged, there's a more logical conclusion. we envisioned different solutions, so having the ideas in our minds not line up isnt terribly surprising. > Use the attached, and then just set uclinux_ram_map.phys = your_address_here > in your setup_arch(). Having weak symbols in drivers that are supposed to > be overriden by the architecture code is just way too backwards for > words. Globals suffice fine for this sort of thing, if you are not going > to go to the effort to pass this information to the driver directly that > is. i was thinking something else, but obviously this is nicer than what i was thinking -mike
On Tue, May 26, 2009 at 01:24:46PM -0400, Mike Frysinger wrote: > On Tue, May 26, 2009 at 13:06, Paul Mundt wrote: > > Use the attached, and then just set uclinux_ram_map.phys = your_address_here > > in your setup_arch(). Having weak symbols in drivers that are supposed to > > be overriden by the architecture code is just way too backwards for > > words. Globals suffice fine for this sort of thing, if you are not going > > to go to the effort to pass this information to the driver directly that > > is. > > i was thinking something else, but obviously this is nicer than what i > was thinking Unfortunately there is the problem that the map driver itself is a tristate, so if this is built as a module, the symbol will not be available to you. On the other hand, if it doesn't need to ever really be a module, converting it to a bool ought to be workable. There are no in-tree users that enable this as a module anyways.
On Tue, May 26, 2009 at 19:19, Paul Mundt wrote: > On Tue, May 26, 2009 at 01:24:46PM -0400, Mike Frysinger wrote: >> On Tue, May 26, 2009 at 13:06, Paul Mundt wrote: >> > Use the attached, and then just set uclinux_ram_map.phys = your_address_here >> > in your setup_arch(). Having weak symbols in drivers that are supposed to >> > be overriden by the architecture code is just way too backwards for >> > words. Globals suffice fine for this sort of thing, if you are not going >> > to go to the effort to pass this information to the driver directly that >> > is. >> >> i was thinking something else, but obviously this is nicer than what i >> was thinking > > Unfortunately there is the problem that the map driver itself is a > tristate, so if this is built as a module, the symbol will not be > available to you. On the other hand, if it doesn't need to ever really be > a module, converting it to a bool ought to be workable. There are no > in-tree users that enable this as a module anyways. weaks would address that, but there is no real point as you say. it isnt like the rootfs image gets loaded as part of the module loading. i'll send a patch for that too. the Blackfin code all depends on CONFIG_MTD_UCLINUX, so the rootfs area wouldnt be setup anyways if it were built as a module ... -mike
Jivin Paul Mundt lays it down ... > On Tue, May 26, 2009 at 01:24:46PM -0400, Mike Frysinger wrote: > > On Tue, May 26, 2009 at 13:06, Paul Mundt wrote: > > > Use the attached, and then just set uclinux_ram_map.phys = your_address_here > > > in your setup_arch(). Having weak symbols in drivers that are supposed to > > > be overriden by the architecture code is just way too backwards for > > > words. Globals suffice fine for this sort of thing, if you are not going > > > to go to the effort to pass this information to the driver directly that > > > is. > > > > i was thinking something else, but obviously this is nicer than what i > > was thinking > > Unfortunately there is the problem that the map driver itself is a > tristate, so if this is built as a module, the symbol will not be > available to you. On the other hand, if it doesn't need to ever really be > a module, converting it to a bool ought to be workable. There are no > in-tree users that enable this as a module anyways. Based on how it relocates the rootfs, I don't believe a module can ever be work, so a boolean would be a better choice IMO, Cheers, Davidm
On Tue, May 26, 2009 at 07:33:16PM -0400, Mike Frysinger wrote: > Due to a processor anomaly (05000263 to be exact), most Blackfin parts > cannot keep the embedded filesystem image directly after the kernel in > RAM. Instead, the filesystem needs to be relocated to the end of memory. > As such, we need to tweak the map addr/size during boot for Blackfin > systems. This can be done in any early arch/board init code. > > Signed-off-by: Mike Frysinger <vapier@gentoo.org> > CC: Paul Mundt <lethal@linux-sh.org> > CC: Greg Ungerer <gerg@uclinux.org> > CC: uclinux-dev@uclinux.org > CC: linux-mtd@lists.infradead.org Signed-off-by: Paul Mundt <lethal@linux-sh.org>
On Tue, May 26, 2009 at 07:33:16PM -0400, Mike Frysinger wrote: > +extern char _ebss; > + Also, it would be nice if the remaining architectures that define _ebss could shove this in their asm/sections.h so we can kill this off from the driver itself.
On Tuesday 26 May 2009 19:46:16 Paul Mundt wrote: > On Tue, May 26, 2009 at 07:33:16PM -0400, Mike Frysinger wrote: > > +extern char _ebss; > > + > > Also, it would be nice if the remaining architectures that define _ebss > could shove this in their asm/sections.h so we can kill this off from the > driver itself. well, everyone would still have to define the _ebss symbol otherwise they'd get an undefined error due to how the map is initialized. -mike
On Tue, May 26, 2009 at 08:46:24PM -0400, Mike Frysinger wrote: > On Tuesday 26 May 2009 19:46:16 Paul Mundt wrote: > > On Tue, May 26, 2009 at 07:33:16PM -0400, Mike Frysinger wrote: > > > +extern char _ebss; > > > + > > > > Also, it would be nice if the remaining architectures that define _ebss > > could shove this in their asm/sections.h so we can kill this off from the > > driver itself. > > well, everyone would still have to define the _ebss symbol otherwise they'd > get an undefined error due to how the map is initialized. Yes, but that is not the issue. The issue is that the section extern has no place in a driver, this is precisely what asm/sections.h is for. Only microblaze and sh define it there today, and there is already inconsistency. Having the others define it for themselves and agreeing on a type would permit us to kill it off from the driver.
diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c index 81756e3..12f822d 100644 --- a/drivers/mtd/maps/uclinux.c +++ b/drivers/mtd/maps/uclinux.c @@ -22,8 +22,11 @@ /****************************************************************************/ +extern char _ebss; + struct map_info uclinux_ram_map = { .name = "RAM", + .phys = (unsigned long)&_ebss, }; struct mtd_info *uclinux_ram_mtdinfo; @@ -55,12 +58,9 @@ static int __init uclinux_mtd_init(void) { struct mtd_info *mtd; struct map_info *mapp; - extern char _ebss; - unsigned long addr = (unsigned long) &_ebss; mapp = &uclinux_ram_map; - mapp->phys = addr; - mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(addr + 8)))); + mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8)))); mapp->bankwidth = 4; printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n",