[U-Boot] [PATCH] ns16550: tegra: Specify debugging serial port at boot.

[U-Boot] [PATCH] ns16550: tegra: Specify debugging serial port at boot.

[Stephen Warren]

From: Doug Anderson <dianders@chromium.org>

This works together with a kernel change that looks at the scratchpad
register to determine which of the many UARTs it should use for early
printing:

http://www.spinics.net/lists/arm-kernel/msg154633.html

While it is unfortunate to need to pass this information in a second way
(it's already in the device tree), this does allow the very early boot
code (decompressing stub and early assembly code) to print to the right
port.

At the moment, I'm adding this to the UART init function. Alternatively,
we could add a more complex patch to key off of the 'console' setting.

Signed-off-by: Doug Anderson <dianders@chromium.org>
[swarren: Limited the change to Tegra platforms]
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/serial/ns16550.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 0c23955..19a28cd 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -62,6 +62,13 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
 	serial_out(0, &com_port->mdr1);
 #endif
 #endif /* CONFIG_OMAP */
+#if defined(CONFIG_TEGRA2)
+	/*
+	 * Put a 'D' in the scratchpad to let the kernel know which UART
+	 * for earlyprintk [D]ebugging.
+	 */
+	serial_out('D', &com_port->spr);
+#endif
 }
 
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
-- 
1.7.0.4

[U-Boot] [PATCH] ns16550: tegra: Specify debugging serial port at boot.

[Doug Anderson]

On Fri, Jan 6, 2012 at 2:51 PM, Stephen Warren <swarren@nvidia.com> wrote:
> From: Doug Anderson <dianders@chromium.org>
>
> This works together with a kernel change that looks at the scratchpad
> register to determine which of the many UARTs it should use for early
> printing:
>
> http://www.spinics.net/lists/arm-kernel/msg154633.html
>
> While it is unfortunate to need to pass this information in a second way
> (it's already in the device tree), this does allow the very early boot
> code (decompressing stub and early assembly code) to print to the right
> port.
>
> At the moment, I'm adding this to the UART init function. Alternatively,
> we could add a more complex patch to key off of the 'console' setting.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> [swarren: Limited the change to Tegra platforms]
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Doug Anderson <dianders@chromium.org>

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Stephen Warren]

On 01/06/2012 05:51 AM, Stephen Warren wrote:
> From: Doug Anderson <dianders@chromium.org>
> 
> This works together with a kernel change that looks at the scratchpad
> register to determine which of the many UARTs it should use for early
> printing:
> 
> http://www.spinics.net/lists/arm-kernel/msg154633.html
> 
> While it is unfortunate to need to pass this information in a second way
> (it's already in the device tree), this does allow the very early boot
> code (decompressing stub and early assembly code) to print to the right
> port.
> 
> At the moment, I'm adding this to the UART init function. Alternatively,
> we could add a more complex patch to key off of the 'console' setting.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> [swarren: Limited the change to Tegra platforms]
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Doug Anderson <dianders@chromium.org>

I noticed this patch isn't applied yet that I can find. Are there any
comments on it; can it be applied? Thanks.

For reference, it's in patchwork at:
http://patchwork.ozlabs.org/patch/134712/

> ---
> drivers/serial/ns16550.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 0c23955..19a28cd 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -62,6 +62,13 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
>  	serial_out(0, &com_port->mdr1);
>  #endif
>  #endif /* CONFIG_OMAP */
> +#if defined(CONFIG_TEGRA2)
> +	/*
> +	 * Put a 'D' in the scratchpad to let the kernel know which UART
> +	 * for earlyprintk [D]ebugging.
> +	 */
> +	serial_out('D', &com_port->spr);
> +#endif
>  }
>  
>  #ifndef CONFIG_NS16550_MIN_FUNCTIONS

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Wolfgang Denk]

Dear Stephen Warren,

In message <4F58F5B8.6070402@wwwdotorg.org> you wrote:
>
> I noticed this patch isn't applied yet that I can find. Are there any
> comments on it; can it be applied? Thanks.
> 
> For reference, it's in patchwork at:
> http://patchwork.ozlabs.org/patch/134712/
> 
> > ---
> > drivers/serial/ns16550.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> > index 0c23955..19a28cd 100644
> > --- a/drivers/serial/ns16550.c
> > +++ b/drivers/serial/ns16550.c
> > @@ -62,6 +62,13 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
> >  	serial_out(0, &com_port->mdr1);
> >  #endif
> >  #endif /* CONFIG_OMAP */
> > +#if defined(CONFIG_TEGRA2)
> > +	/*
> > +	 * Put a 'D' in the scratchpad to let the kernel know which UART
> > +	 * for earlyprintk [D]ebugging.
> > +	 */
> > +	serial_out('D', &com_port->spr);
> > +#endif
> >  }

I don't like to see such highly architecture specific stuff in common
code, especially if it's such a dirty hack like this.

I don't really understand the arguments for the need of this patch
either.  There are standard ways for passing hardware consifuration to
the kernel, and the comment shows that you are aware of these.

Inventing yet another hackish method seems not a good idea to me.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Chapter 1 -- The story so  far:
In the beginning the Universe was created. This has  made  a  lot  of
people very angry and been widely regarded as a bad move.

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Stephen Warren]

On 03/08/2012 11:39 AM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <4F58F5B8.6070402@wwwdotorg.org> you wrote:
>>
>> I noticed this patch isn't applied yet that I can find. Are there any
>> comments on it; can it be applied? Thanks.
>>
>> For reference, it's in patchwork at:
>> http://patchwork.ozlabs.org/patch/134712/
>>
>>> ---
>>> drivers/serial/ns16550.c |    7 +++++++
>>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> index 0c23955..19a28cd 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -62,6 +62,13 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
>>>  	serial_out(0, &com_port->mdr1);
>>>  #endif
>>>  #endif /* CONFIG_OMAP */
>>> +#if defined(CONFIG_TEGRA2)
>>> +	/*
>>> +	 * Put a 'D' in the scratchpad to let the kernel know which UART
>>> +	 * for earlyprintk [D]ebugging.
>>> +	 */
>>> +	serial_out('D', &com_port->spr);
>>> +#endif
>>>  }
> 
> I don't like to see such highly architecture specific stuff in common
> code, especially if it's such a dirty hack like this.

Are there any hooks where we can do the same thing in SoC-specific code?

> I don't really understand the arguments for the need of this patch
> either.  There are standard ways for passing hardware consifuration to
> the kernel, and the comment shows that you are aware of these.
> 
> Inventing yet another hackish method seems not a good idea to me.

The point of this information is to enable the kernel's earlyprintk
support, which runs well before the device tree, or other mechanisms,
are available.

As soon as the regular console, as set by the kernel command-line etc.,
is initialized by the regular "higher level" mechanisms, it takes over
from this earlyprintk code.

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Wolfgang Denk]

Dear Stephen,

In message <4F590B25.8090503@wwwdotorg.org> you wrote:
>
> > I don't like to see such highly architecture specific stuff in common
> > code, especially if it's such a dirty hack like this.
> 
> Are there any hooks where we can do the same thing in SoC-specific code?

Not without additional trickery, but I think this is actually a good
thing.

The method implemented here is but a dirty hack, and should not be
used.

> The point of this information is to enable the kernel's earlyprintk
> support, which runs well before the device tree, or other mechanisms,
> are available.

Sorry, but I don't buy that this is the only possible way to do that.
Or how comes only tegra2 would need that, while all other SoCs and
architectures can do without it?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the beginning there was nothing.
And the Lord said "Let There Be Light!"
And still there was nothing, but at least now you could see it.

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Stephen Warren]

On 03/08/2012 02:29 PM, Wolfgang Denk wrote:
> Dear Stephen,
> 
> In message <4F590B25.8090503@wwwdotorg.org> you wrote:
>>
>>> I don't like to see such highly architecture specific stuff in common
>>> code, especially if it's such a dirty hack like this.
>>
>> Are there any hooks where we can do the same thing in SoC-specific code?
> 
> Not without additional trickery, but I think this is actually a good
> thing.
> 
> The method implemented here is but a dirty hack, and should not be
> used.
> 
>> The point of this information is to enable the kernel's earlyprintk
>> support, which runs well before the device tree, or other mechanisms,
>> are available.
> 
> Sorry, but I don't buy that this is the only possible way to do that.
> Or how comes only tegra2 would need that, while all other SoCs and
> architectures can do without it?

First, OMAP does something very similar; the kernel low-level debug code
looks at UART1's scratch pad register, and derives which UART to use
based on the value stored there. If none of the expected values is
found, it appears to default to UART1.

On Tegra, the UART registers can't be read unless the UART is clocked
and not in reset. So, the Tegra code looks at each UART in the system,
and finds one that's in that state. To cater for the scenario where
multiple UARTs are clocked-and-not-reset, the code also checks whether
the UART scratch register contains 'D' ("D"ebug) so it's sure it picked
the correct one.

So, at leasst OMAP has set precedent here. There may be others; I didn't
check.

Some other SoCs may have only 1 UART and not need to auto-select.

Some other SoCs with multiple UARTs may designate a single specific UART
as the debug port rather than the board designer apparently randomly
picking which one to use. In either of those cases, the kernel's
low-level debug code can simply hard-code the UART address and do
without the hand-shaking.

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Graeme Russ]

Hi Stephen, Wolfgang,

On Fri, Mar 9, 2012 at 8:43 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/08/2012 02:29 PM, Wolfgang Denk wrote:
>> Dear Stephen,
>>
>> In message <4F590B25.8090503@wwwdotorg.org> you wrote:
>>>
>>>> I don't like to see such highly architecture specific stuff in common
>>>> code, especially if it's such a dirty hack like this.
>>>
>>> Are there any hooks where we can do the same thing in SoC-specific code?
>>
>> Not without additional trickery, but I think this is actually a good
>> thing.
>>
>> The method implemented here is but a dirty hack, and should not be
>> used.

INIT_FUNC will resolve this issue long-term - Tegra can just put in:

int tegra_set_debug_port(void)
{
        /*
         * Put a 'D' in the scratchpad to let the kernel know which UART
         * for earlyprintk [D]ebugging.
         */
        serial_out('D', &com_port->spr);

        return 0;
}
INIT_FUNC(set_debug_port, tegra_set_debug_port, *serial_init)

>>> The point of this information is to enable the kernel's earlyprintk
>>> support, which runs well before the device tree, or other mechanisms,
>>> are available.
>>
>> Sorry, but I don't buy that this is the only possible way to do that.
>> Or how comes only tegra2 would need that, while all other SoCs and
>> architectures can do without it?
>
> First, OMAP does something very similar; the kernel low-level debug code
> looks at UART1's scratch pad register, and derives which UART to use
> based on the value stored there. If none of the expected values is
> found, it appears to default to UART1.
>
> On Tegra, the UART registers can't be read unless the UART is clocked
> and not in reset. So, the Tegra code looks at each UART in the system,
> and finds one that's in that state. To cater for the scenario where
> multiple UARTs are clocked-and-not-reset, the code also checks whether
> the UART scratch register contains 'D' ("D"ebug) so it's sure it picked
> the correct one.
>
> So, at leasst OMAP has set precedent here. There may be others; I didn't
> check.
>
> Some other SoCs may have only 1 UART and not need to auto-select.
>
> Some other SoCs with multiple UARTs may designate a single specific UART
> as the debug port rather than the board designer apparently randomly
> picking which one to use. In either of those cases, the kernel's
> low-level debug code can simply hard-code the UART address and do
> without the hand-shaking.

As far as I am aware, earlyprintk happens well before processing the kernel
command line so, IMHO, I don't consider setting up the hardware in order
for the kernel to get some low-level information which cannot be
provided by the command line as a hack. Reading the above, I actually think
it is quite elegant

Regards,

Graeme

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Stephen Warren]

On 03/08/2012 11:39 AM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <4F58F5B8.6070402@wwwdotorg.org> you wrote:
>>
>> I noticed this patch isn't applied yet that I can find. Are there any
>> comments on it; can it be applied? Thanks.
>>
>> For reference, it's in patchwork at:
>> http://patchwork.ozlabs.org/patch/134712/
...
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> @@ -62,6 +62,13 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
>>> +#if defined(CONFIG_TEGRA2)
>>> +	/*
>>> +	 * Put a 'D' in the scratchpad to let the kernel know which UART
>>> +	 * for earlyprintk [D]ebugging.
>>> +	 */
>>> +	serial_out('D', &com_port->spr);
>>> +#endif
> 
> I don't like to see such highly architecture specific stuff in common
> code, especially if it's such a dirty hack like this.

Would it help if we moved the this code to some Tegra-specific file
rather than the common serial port driver?

[U-Boot] ns16550: tegra: Specify debugging serial port at boot.

[Graeme Russ]

Hi Stephen,

On Wed, Mar 14, 2012 at 7:12 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/08/2012 11:39 AM, Wolfgang Denk wrote:
>> Dear Stephen Warren,
>>
>> In message <4F58F5B8.6070402@wwwdotorg.org> you wrote:
>>>
>>> I noticed this patch isn't applied yet that I can find. Are there any
>>> comments on it; can it be applied? Thanks.
>>>
>>> For reference, it's in patchwork at:
>>> http://patchwork.ozlabs.org/patch/134712/
> ...
>>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>>> @@ -62,6 +62,13 @@ void NS16550_init(NS16550_t com_port, int baud_divisor)
>>>> +#if defined(CONFIG_TEGRA2)
>>>> + ? /*
>>>> + ? ?* Put a 'D' in the scratchpad to let the kernel know which UART
>>>> + ? ?* for earlyprintk [D]ebugging.
>>>> + ? ?*/
>>>> + ? serial_out('D', &com_port->spr);
>>>> +#endif
>>
>> I don't like to see such highly architecture specific stuff in common
>> code, especially if it's such a dirty hack like this.
>
> Would it help if we moved the this code to some Tegra-specific file
> rather than the common serial port driver?

Could it be done as the last thig before jumping into the Linux kernel in
arch_preboot_os()

Regards,

Graeme