| Message ID | 20110819110822.GB8918@e102144-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 19, 2011 at 12:08:22PM +0100, Will Deacon wrote: > On Fri, Aug 19, 2011 at 05:59:57AM +0100, Shawn Guo wrote: > > On Thu, Aug 18, 2011 at 05:07:12PM +0100, Will Deacon wrote: > > > I'm not sure that sorting this list in alphabetical order is a good idea. > > > This is a Kconfig choice, so the default value is the first one in the list > > > that has its dependencies satisfied. Therefore, the ordering has a direct > > > impact on the default UART selection. > > > > > I'm unsure that the default UART selection makes much sense here. > > When we build so many platforms into single image, it's hard to say > > which one should be the default. People anyway need to check if the > > the UART selection matches the platform they are debugging on. > > Ok, how about having the default choice as a dummy option which doesn't > correspond to a UART?: > To me, it is a little bit overkilled. Can we just sort them in alphabetical order and let the first one be the winner? We can take this as a reward to the good naming :) > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug > index f23975a..455bc8c 100644 > --- a/arch/arm/Kconfig.debug > +++ b/arch/arm/Kconfig.debug > @@ -65,8 +65,12 @@ config DEBUG_USER > > # These options are only for real kernel hackers who want to get their hands dirty. > config DEBUG_LL > + bool > + > +config DEBUG_LL_UART > bool "Kernel low-level debugging functions" > depends on DEBUG_KERNEL > + select DEBUG_LL if !DEBUG_UART_NONE > help > Say Y here to include definitions of printascii, printch, printhex > in the kernel. This is helpful if you are debugging code that > @@ -74,7 +78,12 @@ config DEBUG_LL > > choice > prompt "Kernel low-level debugging port" > - depends on DEBUG_LL > + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ > + PLAT_SAMSUNG || ARCH_REALVIEW) We will have to list a lot of ARCH/PLAT symbols here. This is what I meant overkilled actually. Regards, Shawn > + default DEBUG_UART_NONE > + > + config DEBUG_UART_NONE > + bool "No UART selected (default)" > > config DEBUG_DC21285_PORT > bool "Kernel low-level debugging messages via footbridge serial port" > > > That way, you really have to select the UART for DEBUG_LL to be enabled. > > > BTW, I just posted one patch for imx based on your series. Are you > > interested in folding it into yours? > > Yep, I'll pick it up once Sascha is happy with it. If we go with the above, > I'm happy to make the necessary changes. >
On Fri, Aug 19, 2011 at 12:37:41PM +0100, Shawn Guo wrote: > On Fri, Aug 19, 2011 at 12:08:22PM +0100, Will Deacon wrote: > > On Fri, Aug 19, 2011 at 05:59:57AM +0100, Shawn Guo wrote: > > > On Thu, Aug 18, 2011 at 05:07:12PM +0100, Will Deacon wrote: > > > > I'm not sure that sorting this list in alphabetical order is a good idea. > > > > This is a Kconfig choice, so the default value is the first one in the list > > > > that has its dependencies satisfied. Therefore, the ordering has a direct > > > > impact on the default UART selection. > > > > > > > I'm unsure that the default UART selection makes much sense here. > > > When we build so many platforms into single image, it's hard to say > > > which one should be the default. People anyway need to check if the > > > the UART selection matches the platform they are debugging on. > > > > Ok, how about having the default choice as a dummy option which doesn't > > correspond to a UART?: > > > To me, it is a little bit overkilled. Can we just sort them in > alphabetical order and let the first one be the winner? We can take > this as a reward to the good naming :) We could do this, I just thought it might be better to force the user to select a UART rather than blindly using the first one in the list. Then again, as Russell pointed out, DEBUG_LL should only be selected if you know what you are doing. > > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug > > index f23975a..455bc8c 100644 > > --- a/arch/arm/Kconfig.debug > > +++ b/arch/arm/Kconfig.debug > > @@ -65,8 +65,12 @@ config DEBUG_USER > > > > # These options are only for real kernel hackers who want to get their hands dirty. > > config DEBUG_LL > > + bool > > + > > +config DEBUG_LL_UART > > bool "Kernel low-level debugging functions" > > depends on DEBUG_KERNEL > > + select DEBUG_LL if !DEBUG_UART_NONE > > help > > Say Y here to include definitions of printascii, printch, printhex > > in the kernel. This is helpful if you are debugging code that > > @@ -74,7 +78,12 @@ config DEBUG_LL > > > > choice > > prompt "Kernel low-level debugging port" > > - depends on DEBUG_LL > > + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ > > + PLAT_SAMSUNG || ARCH_REALVIEW) > > We will have to list a lot of ARCH/PLAT symbols here. This is what > I meant overkilled actually. Ultimately, we will want to have all the platforms using this mechanism so this list of symbols could eventually be removed. I take your point though; so I'll leave the patch series as it is for now. Will
On Fri, 19 Aug 2011, Shawn Guo wrote: > On Fri, Aug 19, 2011 at 12:08:22PM +0100, Will Deacon wrote: > > On Fri, Aug 19, 2011 at 05:59:57AM +0100, Shawn Guo wrote: > > > On Thu, Aug 18, 2011 at 05:07:12PM +0100, Will Deacon wrote: > > > > I'm not sure that sorting this list in alphabetical order is a good idea. > > > > This is a Kconfig choice, so the default value is the first one in the list > > > > that has its dependencies satisfied. Therefore, the ordering has a direct > > > > impact on the default UART selection. > > > > > > > I'm unsure that the default UART selection makes much sense here. > > > When we build so many platforms into single image, it's hard to say > > > which one should be the default. People anyway need to check if the > > > the UART selection matches the platform they are debugging on. > > > > Ok, how about having the default choice as a dummy option which doesn't > > correspond to a UART?: > > > To me, it is a little bit overkilled. Can we just sort them in > alphabetical order and let the first one be the winner? We can take > this as a reward to the good naming :) Just use the alphabetical order, except for the first entry which could be a noop. > > choice > > prompt "Kernel low-level debugging port" > > - depends on DEBUG_LL > > + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ > > + PLAT_SAMSUNG || ARCH_REALVIEW) > > We will have to list a lot of ARCH/PLAT symbols here. This is what > I meant overkilled actually. Agreed. Please avoid the need for such localized long lists of dependencies which are going to create merge conflicts for sure, and which grow into a mess after a while. Nicolas
On Fri, 19 Aug 2011, Will Deacon wrote: > On Fri, Aug 19, 2011 at 12:37:41PM +0100, Shawn Guo wrote: > > On Fri, Aug 19, 2011 at 12:08:22PM +0100, Will Deacon wrote: > > > @@ -74,7 +78,12 @@ config DEBUG_LL > > > > > > choice > > > prompt "Kernel low-level debugging port" > > > - depends on DEBUG_LL > > > + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ > > > + PLAT_SAMSUNG || ARCH_REALVIEW) > > > > We will have to list a lot of ARCH/PLAT symbols here. This is what > > I meant overkilled actually. > > Ultimately, we will want to have all the platforms using this mechanism so > this list of symbols could eventually be removed. I take your point though; > so I'll leave the patch series as it is for now. Well, ultimately I'd like to see something even better than that. We should have the bootloader provide the required information to the kernel for it to be able to send bytes to a debug device. It should be rather simple, especially for a serial UART. All we need in that case is: - physical address of the transmit FIFO register - physical address of the register indicating "FIFO full" with a corresponding bit mask - physical address of the register indicating "FIFO empty" with a corresponding bit mask The bootloader should be able to pass that info into an ATAG or a DT node just fine. But in the mean time, for legacy boards, the best we have is this Kconfig approach. Nicolas
On Fri, Aug 19, 2011 at 11:49:10AM -0400, Nicolas Pitre wrote: > On Fri, 19 Aug 2011, Will Deacon wrote: > > On Fri, Aug 19, 2011 at 12:37:41PM +0100, Shawn Guo wrote: > > > On Fri, Aug 19, 2011 at 12:08:22PM +0100, Will Deacon wrote: > > > > @@ -74,7 +78,12 @@ config DEBUG_LL > > > > > > > > choice > > > > prompt "Kernel low-level debugging port" > > > > - depends on DEBUG_LL > > > > + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ > > > > + PLAT_SAMSUNG || ARCH_REALVIEW) > > > > > > We will have to list a lot of ARCH/PLAT symbols here. This is what > > > I meant overkilled actually. > > > > Ultimately, we will want to have all the platforms using this mechanism so > > this list of symbols could eventually be removed. I take your point though; > > so I'll leave the patch series as it is for now. > > Well, ultimately I'd like to see something even better than that. We > should have the bootloader provide the required information to the > kernel for it to be able to send bytes to a debug device. It should be > rather simple, especially for a serial UART. All we need in that case > is: > > - physical address of the transmit FIFO register > > - physical address of the register indicating "FIFO full" with a > corresponding bit mask > > - physical address of the register indicating "FIFO empty" with a > corresponding bit mask You're missing something there. It's not always about FIFO full and FIFO empty. On some platforms, we want _reliable_ debugging, so the debug stuff waits while CTS is inactive. Plus you need the virtual address too, because the LL debug stuff is there to debug around places like the initial assembly, before C code is setup. Plus its not just about bitmasks, but also about size of access, and polarity of bits. So actually you'd need: - virtual and physical address and size of transmit register - virtual and physical address, mask and value of transmit status - virtual and physical address, mask and value of CTS status but then, we have debugging which doesn't have a physical address associated with it (the jtag stuff) so this doesn't cover the full story either. Not only that of course, but boot loaders should not be passing virtual addresses to the kernel. So all in all, passing this information in from a boot loader is just a plain and simple bad idea.
On Sun, 21 Aug 2011, Russell King - ARM Linux wrote: > On Fri, Aug 19, 2011 at 11:49:10AM -0400, Nicolas Pitre wrote: > > On Fri, 19 Aug 2011, Will Deacon wrote: > > > On Fri, Aug 19, 2011 at 12:37:41PM +0100, Shawn Guo wrote: > > > > On Fri, Aug 19, 2011 at 12:08:22PM +0100, Will Deacon wrote: > > > > > @@ -74,7 +78,12 @@ config DEBUG_LL > > > > > > > > > > choice > > > > > prompt "Kernel low-level debugging port" > > > > > - depends on DEBUG_LL > > > > > + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ > > > > > + PLAT_SAMSUNG || ARCH_REALVIEW) > > > > > > > > We will have to list a lot of ARCH/PLAT symbols here. This is what > > > > I meant overkilled actually. > > > > > > Ultimately, we will want to have all the platforms using this mechanism so > > > this list of symbols could eventually be removed. I take your point though; > > > so I'll leave the patch series as it is for now. > > > > Well, ultimately I'd like to see something even better than that. We > > should have the bootloader provide the required information to the > > kernel for it to be able to send bytes to a debug device. It should be > > rather simple, especially for a serial UART. All we need in that case > > is: > > > > - physical address of the transmit FIFO register > > > > - physical address of the register indicating "FIFO full" with a > > corresponding bit mask > > > > - physical address of the register indicating "FIFO empty" with a > > corresponding bit mask > > You're missing something there. It's not always about FIFO full and FIFO > empty. On some platforms, we want _reliable_ debugging, so the debug > stuff waits while CTS is inactive. OK, let's consider this too. Another register address/bitmask pair. This could be combined into some "OK to transmit" list of such pairs. > Plus you need the virtual address too, because the LL debug stuff is > there to debug around places like the initial assembly, before C code > is setup. The kernel should determine or set up the virtual address by itself. This is obviously not something we want the bootloader to provide. For the same reason, I've discarded the idea that the bootloader could simply have provided via a DT node a small segment of code (it's only a few assembly instructions after all) to drive the serial port because it would require a stable mapping to match that code's idea of the register locations. > Plus its not just about bitmasks, but also about size of access, and > polarity of bits. So actually you'd need: > > - virtual and physical address and size of transmit register > - virtual and physical address, mask and value of transmit status > - virtual and physical address, mask and value of CTS status > > but then, we have debugging which doesn't have a physical address > associated with it (the jtag stuff) so this doesn't cover the full > story either. I'm not trying to cover the full story. This is mainly for 98% of those cases where a plain serial port is used for both the DEBUG_LL _and_ the early output from the decompressor. This is especially important if we want to boot a single zImage on multiple SOCs while preserving the early serial output capability. The alternative is of course to wait for vendors to provide a BIOS-like interface, which is something I know they would be more than happy to do in the near future (they have ... big ideas beyond a generic serial output facility). So I'd much prefer if we came up with a way to preserve as much independence from any such ROM firmware and keep control of the execution environment on Linux's side. Those people with a JTAG debugger and the knowledge to use it really don't need any generic infrastructure to get some early debugging information out. They can reconfigure their kernel and even hack the source to suit their needs. But the people who are going to be the main consumers of a multi-SOC single-binary kernel won't be the ones recompiling their kernel just to provide us with debugging info. Nicolas
On Sun, Aug 21, 2011 at 01:35:33PM -0400, Nicolas Pitre wrote: > On Sun, 21 Aug 2011, Russell King - ARM Linux wrote: > > Plus you need the virtual address too, because the LL debug stuff is > > there to debug around places like the initial assembly, before C code > > is setup. > > The kernel should determine or set up the virtual address by itself. > This is obviously not something we want the bootloader to provide. For > the same reason, I've discarded the idea that the bootloader could > simply have provided via a DT node a small segment of code (it's only a > few assembly instructions after all) to drive the serial port because it > would require a stable mapping to match that code's idea of the register > locations. Yes, and the virtual and physical base addresses are set via the existing macros. To take the virtual address out of that means that we then have to find some way of storing that data - which can't be inside the normal kernel bss or data sections. BSS has not been zeroed at the point where we want working DEBUG_LL stuff. Defining a offset-fixed memory location from the kernel is fragile, and will end up wasting the entire page - which would have to be permanently reserved. This is just getting _idiotic_. There are times when "no" is the word which has to be used, and this is one of them. > I'm not trying to cover the full story. This is mainly for 98% of those > cases where a plain serial port is used for both the DEBUG_LL _and_ the > early output from the decompressor. So yet again we end up with another half baked "solution", which will result in "end-users" being confused because it'll work on some stuff but not on other stuff. That is *no* solution what so ever. > Those people with a JTAG debugger and the knowledge to use it really > don't need any generic infrastructure to get some early debugging > information out. They can reconfigure their kernel and even hack the > source to suit their needs. But the people who are going to be the main > consumers of a multi-SOC single-binary kernel won't be the ones > recompiling their kernel just to provide us with debugging info. That is a stupid argument. The people who need the early console are those bringing up a new board. By the act of being involved in bringing up a new board, they are a developer. They are not a user. They will be having to rebuild the kernel. There is no technical problem here. It's just entirely conceptual, and one of trying to use stuff inappropriately.
On Sun, 21 Aug 2011, Russell King - ARM Linux wrote: > On Sun, Aug 21, 2011 at 01:35:33PM -0400, Nicolas Pitre wrote: > > On Sun, 21 Aug 2011, Russell King - ARM Linux wrote: > > > Plus you need the virtual address too, because the LL debug stuff is > > > there to debug around places like the initial assembly, before C code > > > is setup. > > > > The kernel should determine or set up the virtual address by itself. > > This is obviously not something we want the bootloader to provide. For > > the same reason, I've discarded the idea that the bootloader could > > simply have provided via a DT node a small segment of code (it's only a > > few assembly instructions after all) to drive the serial port because it > > would require a stable mapping to match that code's idea of the register > > locations. > > Yes, and the virtual and physical base addresses are set via the > existing macros. > > To take the virtual address out of that means that we then have to find > some way of storing that data - which can't be inside the normal kernel > bss or data sections. BSS has not been zeroed at the point where we want > working DEBUG_LL stuff. > > Defining a offset-fixed memory location from the kernel is fragile, and > will end up wasting the entire page - which would have to be permanently > reserved. Obviously the way to go is to use a .data variable. And it can be referenced from assembly code in a position independent manner: .macro get_ptr_pic, ptr, symbol b 199f .align 198: .long . .long \symbol 199: adr \ptr, 198b @ get relative address of 198b ldr ip, [\ptr] @ get absolute address of 198b sub ip, ip, \ptr @ offset between abs and rel ldr \ptr, [\ptr, #4] @ absolute address of \symbol sub \ptr, \ptr, ip @ make it relative .endm > This is just getting _idiotic_. There are times when "no" is the word > which has to be used, and this is one of them. You are saying "no" to the possibility of getting rid of the zImage decompressor serial output. And now you're saying "no" to any possibility of keeping it alive in the context of a multi-SOC kernel. Are you also saying "no" to the multi-SOC kernel? If so you'll have to convince many more people than just me. > > I'm not trying to cover the full story. This is mainly for 98% of those > > cases where a plain serial port is used for both the DEBUG_LL _and_ the > > early output from the decompressor. > > So yet again we end up with another half baked "solution", which will > result in "end-users" being confused because it'll work on some stuff > but not on other stuff. The stuff this doesn't work on is not something "end-users" will ever deal with. So this is a non argument. > > Those people with a JTAG debugger and the knowledge to use it really > > don't need any generic infrastructure to get some early debugging > > information out. They can reconfigure their kernel and even hack the > > source to suit their needs. But the people who are going to be the main > > consumers of a multi-SOC single-binary kernel won't be the ones > > recompiling their kernel just to provide us with debugging info. > > That is a stupid argument. The people who need the early console are > those bringing up a new board. By the act of being involved in bringing > up a new board, they are a developer. They are not a user. They will > be having to rebuild the kernel. Please stop thinking in terms of the legacy embedded environment we've been dealing with for the last 15 years. People and even companies with lots of influence are pushing for ARM to become more ubiquitous, and for kernel _binaries_ to be bootable on yet-to-exist future boards unchanged, just like x86 has always done. The near availability of an alternative operating system named after panes of glass is probably not unrelated to this. But, just like on x86, booting a kernel binary (say, from a packaged distro) on a new machine might not exactly boot satisfactorily. And this is likely to be the users who notice it first, in which case having functional early serial output might be pretty handy. > There is no technical problem here. It's just entirely conceptual, > and one of trying to use stuff inappropriately. I'm all ears to your suggestions about using stuff appropriately in the stated context. Nicolas
On Sun, Aug 21, 2011 at 03:02:25PM -0400, Nicolas Pitre wrote: > Obviously the way to go is to use a .data variable. > > And it can be referenced from assembly code in a position independent > manner: > > .macro get_ptr_pic, ptr, symbol > b 199f > .align > 198: .long . > .long \symbol > 199: adr \ptr, 198b @ get relative address of 198b > ldr ip, [\ptr] @ get absolute address of 198b > sub ip, ip, \ptr @ offset between abs and rel > ldr \ptr, [\ptr, #4] @ absolute address of \symbol > sub \ptr, \ptr, ip @ make it relative > .endm And have you checked that you can crap over 'ip' in the early assembly code? There are some (small) places where that register is used, which means you accidentally corrupt that register. > > This is just getting _idiotic_. There are times when "no" is the word > > which has to be used, and this is one of them. > > You are saying "no" to the possibility of getting rid of the zImage > decompressor serial output. > > And now you're saying "no" to any possibility of keeping it alive in the > context of a multi-SOC kernel. The decompressor has nothing to do with this. > Are you also saying "no" to the multi-SOC kernel? If so you'll have to > convince many more people than just me. No, I am saying no to the low level debugging code which *developers* rely upon working being fucked up and rendered useless - and so requiring a new implementation to be written - in the name of multi-SoC support. That is what I'm *strongly* objecting to. The LL debug code is there *explicitly* so that it can be used to debug the early kernel assembly and initial part of the kernel bring-up. It is *NOT* there for USERS. The fact that it was tied into earlyprintk *against my better judgement* and now its wanting to be fucked up so it can't be used for its primary purpose just goes to prove that my original assertions at the time that it was used for early printk have been *proven*. So I'm now saying a very very firm NO to this further creap of it. I am not having its utility destroyed. > But, just like on x86, booting a kernel binary (say, from a packaged > distro) on a new machine might not exactly boot satisfactorily. And > this is likely to be the users who notice it first, in which case having > functional early serial output might be pretty handy. Then provide a *proper* early printk implementation which doesn't involve buggering up assembly whos purpose is to do low level assembly debugging.
On Sun, Aug 21, 2011 at 08:18:35PM +0100, Russell King - ARM Linux wrote: > On Sun, Aug 21, 2011 at 03:02:25PM -0400, Nicolas Pitre wrote: > > Obviously the way to go is to use a .data variable. > > > > And it can be referenced from assembly code in a position independent > > manner: > > > > .macro get_ptr_pic, ptr, symbol > > b 199f > > .align > > 198: .long . > > .long \symbol > > 199: adr \ptr, 198b @ get relative address of 198b > > ldr ip, [\ptr] @ get absolute address of 198b > > sub ip, ip, \ptr @ offset between abs and rel > > ldr \ptr, [\ptr, #4] @ absolute address of \symbol > > sub \ptr, \ptr, ip @ make it relative > > .endm > > And have you checked that you can crap over 'ip' in the early assembly > code? There are some (small) places where that register is used, which > means you accidentally corrupt that register. And further to this, I'll point out that the debugging functions are *explicitly* designed to avoid corrupting any more than just r0-r3 and lr. That's not just the IO functions but also the hex and string printing functions. And the head*.S code is explicitly written to expect r0-r3 to be corrupted - which basically means that no long-term values are held in those registers.
On Sun, 21 Aug 2011, Russell King - ARM Linux wrote: > On Sun, Aug 21, 2011 at 03:02:25PM -0400, Nicolas Pitre wrote: > > Obviously the way to go is to use a .data variable. > > > > And it can be referenced from assembly code in a position independent > > manner: > > > > .macro get_ptr_pic, ptr, symbol > > b 199f > > .align > > 198: .long . > > .long \symbol > > 199: adr \ptr, 198b @ get relative address of 198b > > ldr ip, [\ptr] @ get absolute address of 198b > > sub ip, ip, \ptr @ offset between abs and rel > > ldr \ptr, [\ptr, #4] @ absolute address of \symbol > > sub \ptr, \ptr, ip @ make it relative > > .endm > > And have you checked that you can crap over 'ip' in the early assembly > code? There are some (small) places where that register is used, which > means you accidentally corrupt that register. You are down to details now. I'm still trying to validate concepts. Implementation details such this can be sorted out trivially later. > > > This is just getting _idiotic_. There are times when "no" is the word > > > which has to be used, and this is one of them. > > > > You are saying "no" to the possibility of getting rid of the zImage > > decompressor serial output. > > > > And now you're saying "no" to any possibility of keeping it alive in the > > context of a multi-SOC kernel. > > The decompressor has nothing to do with this. Sure it does, since it relies on early serial output capabilities, just like earlyprintk, or possibly even the senduart code. Solving this once for them all, *appropriately*, would certainly be nice. I would be glad to see this need addressed without making further ugly abuses like the OMAP people did with their OMAP_UART_INFO and the arch/arm/plat-omap/include/plat/uncompress.h abomination. > > Are you also saying "no" to the multi-SOC kernel? If so you'll have to > > convince many more people than just me. > > No, I am saying no to the low level debugging code which *developers* > rely upon working being fucked up and rendered useless - and so requiring > a new implementation to be written - in the name of multi-SoC support. > That is what I'm *strongly* objecting to. I really don't mind leaving that code well alone actually. We can well work something out for earlyprintk and zImage needs first, see how it works, get some confidence in it, and then if that appears to be a worthwhile thing to do then _maybe_ a version of the DEBUG_LL code could be implemented in terms of it. In the meantime, if CONFIG_DEBUG_LL is really really meant to be _only_ for early assembly debugging by seasoned developers then I don't think that expanding its configurability through a Kconfig menu is a good thing. Instead, it should probably be _removed_ from Kconfig entirely. Nicolas
* Nicolas Pitre <nico@fluxnic.net> [110821 22:21]: > On Sun, 21 Aug 2011, Russell King - ARM Linux wrote: > > > > The decompressor has nothing to do with this. > > Sure it does, since it relies on early serial output capabilities, just > like earlyprintk, or possibly even the senduart code. Solving this once > for them all, *appropriately*, would certainly be nice. I would be glad > to see this need addressed without making further ugly abuses like the > OMAP people did with their OMAP_UART_INFO and the > arch/arm/plat-omap/include/plat/uncompress.h abomination. Well the way the debug_ll code is currenty set up just won't scale. The basic problem is that the debug_ll code should be machine specific, not arch specific.. Probably the best long term solution is to set up the debug uart based on DT data and initialize it in the uncompress code. Anybody debugging lower level stuff can certainly patch in the ucompress debug_ll code. Regards, Tony
On Tue, Sep 06, 2011 at 12:28:17PM +0300, Tony Lindgren wrote: > Probably the best long term solution is to set up the debug uart based > on DT data and initialize it in the uncompress code. Anybody debugging > lower level stuff can certainly patch in the ucompress debug_ll code. How do you do that before the MMU has been setup? Are you going to write a DT data parser in pure assembly to extract this information?
* Russell King - ARM Linux <linux@arm.linux.org.uk> [110906 02:04]: > On Tue, Sep 06, 2011 at 12:28:17PM +0300, Tony Lindgren wrote: > > Probably the best long term solution is to set up the debug uart based > > on DT data and initialize it in the uncompress code. Anybody debugging > > lower level stuff can certainly patch in the ucompress debug_ll code. > > How do you do that before the MMU has been setup? Are you going to > write a DT data parser in pure assembly to extract this information? Don't we already have that with ATAGs to DT support in the DT append patches? At least it calls fdt_setprop so calling fdt_getprop should also work. The patch I'm talking about is: http://permalink.gmane.org/gmane.linux.drivers.devicetree/5938 Tony
On Tue, Sep 06, 2011 at 03:27:03AM -0700, Tony Lindgren wrote: > * Russell King - ARM Linux <linux@arm.linux.org.uk> [110906 02:04]: > > On Tue, Sep 06, 2011 at 12:28:17PM +0300, Tony Lindgren wrote: > > > Probably the best long term solution is to set up the debug uart based > > > on DT data and initialize it in the uncompress code. Anybody debugging > > > lower level stuff can certainly patch in the ucompress debug_ll code. > > > > How do you do that before the MMU has been setup? Are you going to > > write a DT data parser in pure assembly to extract this information? > > Don't we already have that with ATAGs to DT support in the DT append > patches? At least it calls fdt_setprop so calling fdt_getprop should > also work. The patch I'm talking about is: > > http://permalink.gmane.org/gmane.linux.drivers.devicetree/5938 Let me reiterate. How are you going to do that before the MMU has been setup. I'll give you a hint: in the kernel (not the boot loader) you can't call *ANY* C code until you have the MMU enabled. One of the points of the *LOW LEVEL* debug code is so you can debug the low level assembly in the head*.S files before the MMU has been enabled. That means you can't call the C function "fdt_getprop" to get the parameters. So, should we have a chunk of assembly code to enable the MMU so that we can run fdt_getprop() to get the parameters for the LL debug code, turn the MMU off, and then run through the head.S code to enable the MMU for the kernel proper? If you think that's silly, you're starting to get the picture I've had for years about all this bastardization of the LL debug code to solve problems beyond what it was originally intended for - which, again, was to debug the head.S code. Personally, I think this whole thing is getting impractical for the use which people are trying to stretch it too - it's trying to be used to solve the "how do we get console output before the console is up" problem as well as the "how do we debug the low level assembly code". With the advent of multi-platform kernels, the two requirements have been well proving to be mutually exclusive.
* Russell King - ARM Linux <linux@arm.linux.org.uk> [110906 03:18]: > On Tue, Sep 06, 2011 at 03:27:03AM -0700, Tony Lindgren wrote: > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [110906 02:04]: > > > On Tue, Sep 06, 2011 at 12:28:17PM +0300, Tony Lindgren wrote: > > > > Probably the best long term solution is to set up the debug uart based > > > > on DT data and initialize it in the uncompress code. Anybody debugging > > > > lower level stuff can certainly patch in the ucompress debug_ll code. > > > > > > How do you do that before the MMU has been setup? Are you going to > > > write a DT data parser in pure assembly to extract this information? > > > > Don't we already have that with ATAGs to DT support in the DT append > > patches? At least it calls fdt_setprop so calling fdt_getprop should > > also work. The patch I'm talking about is: > > > > http://permalink.gmane.org/gmane.linux.drivers.devicetree/5938 > > Let me reiterate. How are you going to do that before the MMU has been > setup. > > I'll give you a hint: in the kernel (not the boot loader) you can't > call *ANY* C code until you have the MMU enabled. One of the points > of the *LOW LEVEL* debug code is so you can debug the low level > assembly in the head*.S files before the MMU has been enabled. > > That means you can't call the C function "fdt_getprop" to get the > parameters. I suggested we set it up in the uncompress code and pass the debug_ll configuration to the kernel. This only leaves out the debug_ll code to debug the uncompress code, which can be patched in as needed. > So, should we have a chunk of assembly code to enable the MMU so that > we can run fdt_getprop() to get the parameters for the LL debug code, > turn the MMU off, and then run through the head.S code to enable the > MMU for the kernel proper? If you think that's silly, you're starting > to get the picture I've had for years about all this bastardization of > the LL debug code to solve problems beyond what it was originally > intended for - which, again, was to debug the head.S code. That's not needed if we rely on the uncompress code to set it up. > Personally, I think this whole thing is getting impractical for the > use which people are trying to stretch it too - it's trying to be used > to solve the "how do we get console output before the console is up" > problem as well as the "how do we debug the low level assembly code". > > With the advent of multi-platform kernels, the two requirements have > been well proving to be mutually exclusive. Yes I agree. As an alternative, how about we drop all the debug_ll code and allow people to patch it in as needed? Most users could get by with early_printk based setup that sets up the machine specific configuration fairly early instead of relying on the debug_ll functions. And of course the init order needs to be sane so not much can go wrong before that. Regards, Tony
On Tue, Sep 06, 2011 at 04:01:43AM -0700, Tony Lindgren wrote: > Yes I agree. As an alternative, how about we drop all the debug_ll code > and allow people to patch it in as needed? Given all this argument over it, I'm mindful to rip out all the debug_ll code and keep it permanently as an offline patch to prevent further abuse. It means that people will have to individually write their own accessors rather than having the utility of it being merged in the kernel, but I'm sick of trying to stop the feature creap destroying its utility.
On Tue, Sep 06, 2011 at 12:07:26PM +0100, Russell King - ARM Linux wrote: > On Tue, Sep 06, 2011 at 04:01:43AM -0700, Tony Lindgren wrote: > > Yes I agree. As an alternative, how about we drop all the debug_ll code > > and allow people to patch it in as needed? > > Given all this argument over it, I'm mindful to rip out all the debug_ll > code and keep it permanently as an offline patch to prevent further abuse. Can you please just nack the changes and keep DEBUG_LL as is? You'll have my and IIRC Sascha's support for it. Best regards Uwe
diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug index f23975a..455bc8c 100644 --- a/arch/arm/Kconfig.debug +++ b/arch/arm/Kconfig.debug @@ -65,8 +65,12 @@ config DEBUG_USER # These options are only for real kernel hackers who want to get their hands dirty. config DEBUG_LL + bool + +config DEBUG_LL_UART bool "Kernel low-level debugging functions" depends on DEBUG_KERNEL + select DEBUG_LL if !DEBUG_UART_NONE help Say Y here to include definitions of printascii, printch, printhex in the kernel. This is helpful if you are debugging code that @@ -74,7 +78,12 @@ config DEBUG_LL choice prompt "Kernel low-level debugging port" - depends on DEBUG_LL + depends on DEBUG_LL_UART && (FOOTBRIDGE || ARCH_CLPS711X || \ + PLAT_SAMSUNG || ARCH_REALVIEW) + default DEBUG_UART_NONE + + config DEBUG_UART_NONE + bool "No UART selected (default)" config DEBUG_DC21285_PORT bool "Kernel low-level debugging messages via footbridge serial port"