Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1416314 > unrolled thread

[PATCH] ARM: at91: debug: use DEBUG_UART_VIRT

Started byAlexandre Belloni <alexandre.belloni@free-electrons.com>
First post2016-06-07 17:30 +0200
Last post2016-06-10 17:20 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-07 17:30 +0200
    Re: [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-06-07 17:50 +0200
      Re: [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-07 18:30 +0200
        Re: [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-06-08 09:20 +0200
          Re: [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-06-08 10:20 +0200
    Re: [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-10 17:20 +0200

#1416314 — [PATCH] ARM: at91: debug: use DEBUG_UART_VIRT

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-06-07 17:30 +0200
Subject[PATCH] ARM: at91: debug: use DEBUG_UART_VIRT
Message-ID<rHr74-4oQ-19@gated-at.bofh.it>
AT91 still uses an offset (0x0100 0000) from the physical address to map
the debug UART. This is unfortunate as for some platforms (sama5d3 and
earlier), it ends up in the PCI zone and PCI is enabled in multi_v7.
Switch to DEBUG_UART_VIRT to solve that.

Tested on sama5d3 and 9g20.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/Kconfig.debug        | 11 ++++++-----
 arch/arm/include/debug/at91.S | 10 +---------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 19a3dcf5eb2e..2609491f4ab1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -117,15 +117,15 @@ choice
 		  their output to the serial port on atmel devices.
 
 		  SOC                  DEBUG_UART_PHYS   DEBUG_UART_VIRT  PORT
-		  rm9200, 9260/9g20,   0xfffff200        0xfefff200       DBGU
+		  rm9200, 9260/9g20,   0xfffff200        0xf8fff200       DBGU
 		  9261/9g10, 9rl
-		  9263, 9g45, sama5d3  0xffffee00        0xfeffee00       DBGU
+		  9263, 9g45, sama5d3  0xffffee00        0xf8ffee00       DBGU
 		  sama5d4              0xfc00c000        0xfb00c000       USART3
 		  sama5d4              0xfc069000        0xfb069000       DBGU
 		  sama5d2              0xf8020000        0xf7020000       UART1
 
-		  Please adjust DEBUG_UART_PHYS configuration options based on
-		  your needs.
+		  Please adjust DEBUG_UART_PHYS and DEBUG_UART_VIRT
+		  configuration options based on your needs.
 
 	config DEBUG_BCM2835
 		bool "Kernel low-level debugging on BCM2835 PL011 UART"
@@ -1627,7 +1627,8 @@ config DEBUG_UART_VIRT
 		DEBUG_QCOM_UARTDM || DEBUG_S3C24XX_UART || \
 		DEBUG_S3C64XX_UART || \
 		DEBUG_BCM63XX_UART || DEBUG_ASM9260_UART || \
-		DEBUG_SIRFSOC_UART || DEBUG_DIGICOLOR_UA0
+		DEBUG_SIRFSOC_UART || DEBUG_DIGICOLOR_UA0 || \
+		DEBUG_AT91_UART
 
 config DEBUG_UART_8250_SHIFT
 	int "Register offset shift for the 8250 debug UART"
diff --git a/arch/arm/include/debug/at91.S b/arch/arm/include/debug/at91.S
index d4ae3b8e2426..0098401e5aeb 100644
--- a/arch/arm/include/debug/at91.S
+++ b/arch/arm/include/debug/at91.S
@@ -9,14 +9,6 @@
  *
 */
 
-#ifdef CONFIG_MMU
-#define AT91_IO_P2V(x) ((x) - 0x01000000)
-#else
-#define AT91_IO_P2V(x) (x)
-#endif
-
-#define AT91_DEBUG_UART_VIRT AT91_IO_P2V(CONFIG_DEBUG_UART_PHYS)
-
 #define AT91_DBGU_SR		(0x14)	/* Status Register */
 #define AT91_DBGU_THR		(0x1c)	/* Transmitter Holding Register */
 #define AT91_DBGU_TXRDY		(1 << 1)	/* Transmitter Ready */
@@ -24,7 +16,7 @@
 
 	.macro	addruart, rp, rv, tmp
 	ldr	\rp, =CONFIG_DEBUG_UART_PHYS		@ System peripherals (phys address)
-	ldr	\rv, =AT91_DEBUG_UART_VIRT		@ System peripherals (virt address)
+	ldr	\rv, =CONFIG_DEBUG_UART_VIRT		@ System peripherals (virt address)
 	.endm
 
 	.macro	senduart,rd,rx
-- 
2.8.1

[toc] | [next] | [standalone]

#1416333

FromNicolas Ferre <nicolas.ferre@atmel.com>
Date2016-06-07 17:50 +0200
Message-ID<rHrqp-4vk-9@gated-at.bofh.it>
In reply to#1416314
Le 07/06/2016 17:24, Alexandre Belloni a écrit :
> AT91 still uses an offset (0x0100 0000) from the physical address to map
> the debug UART. This is unfortunate as for some platforms (sama5d3 and
> earlier), it ends up in the PCI zone and PCI is enabled in multi_v7.
> Switch to DEBUG_UART_VIRT to solve that.
> 
> Tested on sama5d3 and 9g20.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

People using their old defconfigs must pay attention to this change...
but it's true that it's a debug configuration anyway...

> ---
>  arch/arm/Kconfig.debug        | 11 ++++++-----
>  arch/arm/include/debug/at91.S | 10 +---------
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 19a3dcf5eb2e..2609491f4ab1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -117,15 +117,15 @@ choice
>  		  their output to the serial port on atmel devices.
>  
>  		  SOC                  DEBUG_UART_PHYS   DEBUG_UART_VIRT  PORT
> -		  rm9200, 9260/9g20,   0xfffff200        0xfefff200       DBGU
> +		  rm9200, 9260/9g20,   0xfffff200        0xf8fff200       DBGU
>  		  9261/9g10, 9rl
> -		  9263, 9g45, sama5d3  0xffffee00        0xfeffee00       DBGU
> +		  9263, 9g45, sama5d3  0xffffee00        0xf8ffee00       DBGU
>  		  sama5d4              0xfc00c000        0xfb00c000       USART3
>  		  sama5d4              0xfc069000        0xfb069000       DBGU
>  		  sama5d2              0xf8020000        0xf7020000       UART1
>  
> -		  Please adjust DEBUG_UART_PHYS configuration options based on
> -		  your needs.
> +		  Please adjust DEBUG_UART_PHYS and DEBUG_UART_VIRT
> +		  configuration options based on your needs.
>  
>  	config DEBUG_BCM2835
>  		bool "Kernel low-level debugging on BCM2835 PL011 UART"
> @@ -1627,7 +1627,8 @@ config DEBUG_UART_VIRT
>  		DEBUG_QCOM_UARTDM || DEBUG_S3C24XX_UART || \
>  		DEBUG_S3C64XX_UART || \
>  		DEBUG_BCM63XX_UART || DEBUG_ASM9260_UART || \
> -		DEBUG_SIRFSOC_UART || DEBUG_DIGICOLOR_UA0
> +		DEBUG_SIRFSOC_UART || DEBUG_DIGICOLOR_UA0 || \
> +		DEBUG_AT91_UART
>  
>  config DEBUG_UART_8250_SHIFT
>  	int "Register offset shift for the 8250 debug UART"
> diff --git a/arch/arm/include/debug/at91.S b/arch/arm/include/debug/at91.S
> index d4ae3b8e2426..0098401e5aeb 100644
> --- a/arch/arm/include/debug/at91.S
> +++ b/arch/arm/include/debug/at91.S
> @@ -9,14 +9,6 @@
>   *
>  */
>  
> -#ifdef CONFIG_MMU
> -#define AT91_IO_P2V(x) ((x) - 0x01000000)
> -#else
> -#define AT91_IO_P2V(x) (x)
> -#endif
> -
> -#define AT91_DEBUG_UART_VIRT AT91_IO_P2V(CONFIG_DEBUG_UART_PHYS)
> -
>  #define AT91_DBGU_SR		(0x14)	/* Status Register */
>  #define AT91_DBGU_THR		(0x1c)	/* Transmitter Holding Register */
>  #define AT91_DBGU_TXRDY		(1 << 1)	/* Transmitter Ready */
> @@ -24,7 +16,7 @@
>  
>  	.macro	addruart, rp, rv, tmp
>  	ldr	\rp, =CONFIG_DEBUG_UART_PHYS		@ System peripherals (phys address)
> -	ldr	\rv, =AT91_DEBUG_UART_VIRT		@ System peripherals (virt address)
> +	ldr	\rv, =CONFIG_DEBUG_UART_VIRT		@ System peripherals (virt address)

Shouldn't we protect the use of this defined value with some
#warning "Beware the value CONFIG_DEBUG_UART_VIRT haven't been defined:
is it intentional"
or even #error?

or something like that?

>  	.endm
>  
>  	.macro	senduart,rd,rx
> 


-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1416393

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-06-07 18:30 +0200
Message-ID<rHs38-4Y1-39@gated-at.bofh.it>
In reply to#1416333
On 07/06/2016 at 17:48:21 +0200, Nicolas Ferre wrote :
> Le 07/06/2016 17:24, Alexandre Belloni a écrit :
> > AT91 still uses an offset (0x0100 0000) from the physical address to map
> > the debug UART. This is unfortunate as for some platforms (sama5d3 and
> > earlier), it ends up in the PCI zone and PCI is enabled in multi_v7.
> > Switch to DEBUG_UART_VIRT to solve that.
> > 
> > Tested on sama5d3 and 9g20.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> People using their old defconfigs must pay attention to this change...
> but it's true that it's a debug configuration anyway...
> 

I really doubt people are letting DEBUG_LL enabled in a production
kernel...

> > diff --git a/arch/arm/include/debug/at91.S b/arch/arm/include/debug/at91.S
> > index d4ae3b8e2426..0098401e5aeb 100644
> > --- a/arch/arm/include/debug/at91.S
> > +++ b/arch/arm/include/debug/at91.S
> > @@ -9,14 +9,6 @@
> >   *
> >  */
> >  
> > -#ifdef CONFIG_MMU
> > -#define AT91_IO_P2V(x) ((x) - 0x01000000)
> > -#else
> > -#define AT91_IO_P2V(x) (x)
> > -#endif
> > -
> > -#define AT91_DEBUG_UART_VIRT AT91_IO_P2V(CONFIG_DEBUG_UART_PHYS)
> > -
> >  #define AT91_DBGU_SR		(0x14)	/* Status Register */
> >  #define AT91_DBGU_THR		(0x1c)	/* Transmitter Holding Register */
> >  #define AT91_DBGU_TXRDY		(1 << 1)	/* Transmitter Ready */
> > @@ -24,7 +16,7 @@
> >  
> >  	.macro	addruart, rp, rv, tmp
> >  	ldr	\rp, =CONFIG_DEBUG_UART_PHYS		@ System peripherals (phys address)
> > -	ldr	\rv, =AT91_DEBUG_UART_VIRT		@ System peripherals (virt address)
> > +	ldr	\rv, =CONFIG_DEBUG_UART_VIRT		@ System peripherals (virt address)
> 
> Shouldn't we protect the use of this defined value with some
> #warning "Beware the value CONFIG_DEBUG_UART_VIRT haven't been defined:
> is it intentional"
> or even #error?
> 
> or something like that?
> 

Well, those that are using this feature are supposed to know what they
are doing. There is now protection for CONFIG_DEBUG_UART_PHYS anyway.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1416952

FromNicolas Ferre <nicolas.ferre@atmel.com>
Date2016-06-08 09:20 +0200
Message-ID<rHFWp-5tG-13@gated-at.bofh.it>
In reply to#1416393
Le 07/06/2016 18:23, Alexandre Belloni a écrit :
> On 07/06/2016 at 17:48:21 +0200, Nicolas Ferre wrote :
>> Le 07/06/2016 17:24, Alexandre Belloni a écrit :
>>> AT91 still uses an offset (0x0100 0000) from the physical address to map
>>> the debug UART. This is unfortunate as for some platforms (sama5d3 and
>>> earlier), it ends up in the PCI zone and PCI is enabled in multi_v7.
>>> Switch to DEBUG_UART_VIRT to solve that.
>>>
>>> Tested on sama5d3 and 9g20.
>>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>
>> People using their old defconfigs must pay attention to this change...
>> but it's true that it's a debug configuration anyway...
>>
> 
> I really doubt people are letting DEBUG_LL enabled in a production
> kernel...
> 
>>> diff --git a/arch/arm/include/debug/at91.S b/arch/arm/include/debug/at91.S
>>> index d4ae3b8e2426..0098401e5aeb 100644
>>> --- a/arch/arm/include/debug/at91.S
>>> +++ b/arch/arm/include/debug/at91.S
>>> @@ -9,14 +9,6 @@
>>>   *
>>>  */
>>>  
>>> -#ifdef CONFIG_MMU
>>> -#define AT91_IO_P2V(x) ((x) - 0x01000000)
>>> -#else
>>> -#define AT91_IO_P2V(x) (x)
>>> -#endif
>>> -
>>> -#define AT91_DEBUG_UART_VIRT AT91_IO_P2V(CONFIG_DEBUG_UART_PHYS)
>>> -
>>>  #define AT91_DBGU_SR		(0x14)	/* Status Register */
>>>  #define AT91_DBGU_THR		(0x1c)	/* Transmitter Holding Register */
>>>  #define AT91_DBGU_TXRDY		(1 << 1)	/* Transmitter Ready */
>>> @@ -24,7 +16,7 @@
>>>  
>>>  	.macro	addruart, rp, rv, tmp
>>>  	ldr	\rp, =CONFIG_DEBUG_UART_PHYS		@ System peripherals (phys address)
>>> -	ldr	\rv, =AT91_DEBUG_UART_VIRT		@ System peripherals (virt address)
>>> +	ldr	\rv, =CONFIG_DEBUG_UART_VIRT		@ System peripherals (virt address)
>>
>> Shouldn't we protect the use of this defined value with some
>> #warning "Beware the value CONFIG_DEBUG_UART_VIRT haven't been defined:
>> is it intentional"
>> or even #error?
>>
>> or something like that?
>>
> 
> Well, those that are using this feature are supposed to know what they
> are doing. There is now protection for CONFIG_DEBUG_UART_PHYS anyway.

So, I know that developers are sensible people and know what they're
doing but still... what about having a protection for both anyway...
instead of silently crashing at runtime?

Bye,
-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1417030

FromNicolas Ferre <nicolas.ferre@atmel.com>
Date2016-06-08 10:20 +0200
Message-ID<rHGSt-685-9@gated-at.bofh.it>
In reply to#1416952
Le 08/06/2016 09:17, Nicolas Ferre a écrit :
> Le 07/06/2016 18:23, Alexandre Belloni a écrit :
>> On 07/06/2016 at 17:48:21 +0200, Nicolas Ferre wrote :
>>> Le 07/06/2016 17:24, Alexandre Belloni a écrit :
>>>> AT91 still uses an offset (0x0100 0000) from the physical address to map
>>>> the debug UART. This is unfortunate as for some platforms (sama5d3 and
>>>> earlier), it ends up in the PCI zone and PCI is enabled in multi_v7.
>>>> Switch to DEBUG_UART_VIRT to solve that.
>>>>
>>>> Tested on sama5d3 and 9g20.
>>>>
>>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>>
>>> People using their old defconfigs must pay attention to this change...
>>> but it's true that it's a debug configuration anyway...
>>>
>>
>> I really doubt people are letting DEBUG_LL enabled in a production
>> kernel...
>>
>>>> diff --git a/arch/arm/include/debug/at91.S b/arch/arm/include/debug/at91.S
>>>> index d4ae3b8e2426..0098401e5aeb 100644
>>>> --- a/arch/arm/include/debug/at91.S
>>>> +++ b/arch/arm/include/debug/at91.S
>>>> @@ -9,14 +9,6 @@
>>>>   *
>>>>  */
>>>>  
>>>> -#ifdef CONFIG_MMU
>>>> -#define AT91_IO_P2V(x) ((x) - 0x01000000)
>>>> -#else
>>>> -#define AT91_IO_P2V(x) (x)
>>>> -#endif
>>>> -
>>>> -#define AT91_DEBUG_UART_VIRT AT91_IO_P2V(CONFIG_DEBUG_UART_PHYS)
>>>> -
>>>>  #define AT91_DBGU_SR		(0x14)	/* Status Register */
>>>>  #define AT91_DBGU_THR		(0x1c)	/* Transmitter Holding Register */
>>>>  #define AT91_DBGU_TXRDY		(1 << 1)	/* Transmitter Ready */
>>>> @@ -24,7 +16,7 @@
>>>>  
>>>>  	.macro	addruart, rp, rv, tmp
>>>>  	ldr	\rp, =CONFIG_DEBUG_UART_PHYS		@ System peripherals (phys address)
>>>> -	ldr	\rv, =AT91_DEBUG_UART_VIRT		@ System peripherals (virt address)
>>>> +	ldr	\rv, =CONFIG_DEBUG_UART_VIRT		@ System peripherals (virt address)
>>>
>>> Shouldn't we protect the use of this defined value with some
>>> #warning "Beware the value CONFIG_DEBUG_UART_VIRT haven't been defined:
>>> is it intentional"
>>> or even #error?
>>>
>>> or something like that?
>>>
>>
>> Well, those that are using this feature are supposed to know what they
>> are doing. There is now protection for CONFIG_DEBUG_UART_PHYS anyway.
> 
> So, I know that developers are sensible people and know what they're
> doing but still... what about having a protection for both anyway...
> instead of silently crashing at runtime?

After discussion with you on IRC, it seems to be not so easy to do and
can harm the multi_v7 early debug facilities.

So, for this patch:
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

It's true anyway that putting the proper values for these config options
is not so easy (reading the help for CONFIG_DEBUG_AT91_UART)... And
enhancing this situation if far beyond the scope of this patch.

Thanks. Bye,
-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1419514

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-06-10 17:20 +0200
Message-ID<rIwo2-6dF-11@gated-at.bofh.it>
In reply to#1416314
On 07/06/2016 at 17:24:39 +0200, Alexandre Belloni wrote :
> AT91 still uses an offset (0x0100 0000) from the physical address to map
> the debug UART. This is unfortunate as for some platforms (sama5d3 and
> earlier), it ends up in the PCI zone and PCI is enabled in multi_v7.
> Switch to DEBUG_UART_VIRT to solve that.
> 
> Tested on sama5d3 and 9g20.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  arch/arm/Kconfig.debug        | 11 ++++++-----
>  arch/arm/include/debug/at91.S | 10 +---------
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
Applied now.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web