[1/3] ARM: debug: use kconfig choice for selecting DEBUG_LL UART
diff mbox

Message ID 20110819110822.GB8918@e102144-lin.cambridge.arm.com
State New, archived
Headers show

Commit Message

Will Deacon Aug. 19, 2011, 11:08 a.m. UTC
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?:



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.

Cheers,

Will

Comments

Shawn Guo Aug. 19, 2011, 11:37 a.m. UTC | #1
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.
>
Will Deacon Aug. 19, 2011, 12:32 p.m. UTC | #2
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
Nicolas Pitre Aug. 19, 2011, 2:54 p.m. UTC | #3
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
Nicolas Pitre Aug. 19, 2011, 3:49 p.m. UTC | #4
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
Russell King - ARM Linux Aug. 21, 2011, 9:14 a.m. UTC | #5
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.
Nicolas Pitre Aug. 21, 2011, 5:35 p.m. UTC | #6
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
Russell King - ARM Linux Aug. 21, 2011, 6:26 p.m. UTC | #7
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.
Nicolas Pitre Aug. 21, 2011, 7:02 p.m. UTC | #8
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
Russell King - ARM Linux Aug. 21, 2011, 7:18 p.m. UTC | #9
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.
Russell King - ARM Linux Aug. 21, 2011, 7:22 p.m. UTC | #10
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.
Nicolas Pitre Aug. 21, 2011, 7:53 p.m. UTC | #11
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
Tony Lindgren Sept. 6, 2011, 9:28 a.m. UTC | #12
* 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
Russell King - ARM Linux Sept. 6, 2011, 9:37 a.m. UTC | #13
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?
Tony Lindgren Sept. 6, 2011, 10:27 a.m. UTC | #14
* 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
Russell King - ARM Linux Sept. 6, 2011, 10:52 a.m. UTC | #15
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.
Tony Lindgren Sept. 6, 2011, 11:01 a.m. UTC | #16
* 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
Russell King - ARM Linux Sept. 6, 2011, 11:07 a.m. UTC | #17
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.
Uwe Kleine-König Sept. 6, 2011, 7:45 p.m. UTC | #18
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

Patch
diff mbox

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"