[PATCH v1] ARM: dts: sun7i: provide ramoops region on BananaPi

[PATCH v1] ARM: dts: sun7i: provide ramoops region on BananaPi

[Oleksij Rempel]

This should help provide useful debug information on remote
targets without UART.

Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-bananapi.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
index ed2f35a..38923bf 100644
--- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
+++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
@@ -63,6 +63,22 @@
 		stdout-path = "serial0:115200n8";
 	};
 
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		ramoops@7fb6a000 {
+			compatible = "ramoops";
+			reg = <0x7fb6a000 0x100000>;
+			ecc-size	= <16>;
+			record-size	= <0x00020000>;
+			console-size	= <0x00020000>;
+			ftrace-size	= <0x00020000>;
+			pmsg-size	= <0x00020000>;
+		};
+	};
+
 	leds {
 		compatible = "gpio-leds";
 		pinctrl-names = "default";
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] ARM: dts: sun7i: provide ramoops region on BananaPi

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Hi Oleksij,

On Wed, Jun 21, 2017 at 07:10:17PM +0200, Oleksij Rempel wrote:
> This should help provide useful debug information on remote
> targets without UART.
> 
> Signed-off-by: Oleksij Rempel <linux-YEK0n+YFykbzxQdaRaTXBw@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun7i-a20-bananapi.dts | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> index ed2f35a..38923bf 100644
> --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> @@ -63,6 +63,22 @@
>  		stdout-path = "serial0:115200n8";
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		ramoops@7fb6a000 {
> +			compatible = "ramoops";
> +			reg = <0x7fb6a000 0x100000>;
> +			ecc-size	= <16>;
> +			record-size	= <0x00020000>;
> +			console-size	= <0x00020000>;
> +			ftrace-size	= <0x00020000>;
> +			pmsg-size	= <0x00020000>;
> +		};
> +	};
> +

I'm a bit skeptical about this one.

First, there's nothing specific to the bananapi, but every one will
want to have different sizes here, or different parameters.

Since using the kernel parameters is also an option, I'd rather
document how to do that with the parameters.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v1] ARM: dts: sun7i: provide ramoops region on BananaPi

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 2623 bytes --]

Am 22.06.2017 um 08:16 schrieb Maxime Ripard:
> Hi Oleksij,
> 
> On Wed, Jun 21, 2017 at 07:10:17PM +0200, Oleksij Rempel wrote:
>> This should help provide useful debug information on remote
>> targets without UART.
>>
>> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
>> ---
>>  arch/arm/boot/dts/sun7i-a20-bananapi.dts | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
>> index ed2f35a..38923bf 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
>> @@ -63,6 +63,22 @@
>>  		stdout-path = "serial0:115200n8";
>>  	};
>>  
>> +	reserved-memory {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges;
>> +
>> +		ramoops@7fb6a000 {
>> +			compatible = "ramoops";
>> +			reg = <0x7fb6a000 0x100000>;
>> +			ecc-size	= <16>;
>> +			record-size	= <0x00020000>;
>> +			console-size	= <0x00020000>;
>> +			ftrace-size	= <0x00020000>;
>> +			pmsg-size	= <0x00020000>;
>> +		};
>> +	};
>> +
> 
> I'm a bit skeptical about this one.

I can understand your concern.

> First, there's nothing specific to the bananapi,

It is specific to memory size, bootloader used for the board and in some
cases security co processor. Any thing else I forgot?

> but every one will> want to have different sizes here, or different parameters.

I don't thing every one need different configurations. I assume current
parameters are good enough for most users. But if users need some thing
different they should be overwritten by kernel boot args.

May be, DTS should provide some generic/ramoops area and let
distributions decide how to use it? For example compile kernel with some
defaults for ramoops.

> Since using the kernel parameters is also an option, I'd rather
> document how to do that with the parameters.

Here are my arguments to do this in DTS:
- some issues are hardly reproducible and it is good to catch it as soon
as possible. So it should be enabled before first problem will happen.
- my previous experience with pstore was a bit disappointing, it didn't
worked on SMP system correctly. So the test coverage seems to be
minimal. Probably because it is not enabled by default.
- at least barebox can extract some postmortem information from not
booting system. Bootloader and Kernel need to use same parameters,
devicetree seems to be perfect here.
- distributions should be able to use this per default as soon as this
option is available.

-- 
Regards,
Oleksij


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] ARM: dts: sun7i: provide ramoops region on BananaPi

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4180 bytes --]

On Thu, Jun 22, 2017 at 08:40:44AM +0200, Oleksij Rempel wrote:
> Am 22.06.2017 um 08:16 schrieb Maxime Ripard:
> > Hi Oleksij,
> > 
> > On Wed, Jun 21, 2017 at 07:10:17PM +0200, Oleksij Rempel wrote:
> >> This should help provide useful debug information on remote
> >> targets without UART.
> >>
> >> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
> >> ---
> >>  arch/arm/boot/dts/sun7i-a20-bananapi.dts | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> >> index ed2f35a..38923bf 100644
> >> --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> >> +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> >> @@ -63,6 +63,22 @@
> >>  		stdout-path = "serial0:115200n8";
> >>  	};
> >>  
> >> +	reserved-memory {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <1>;
> >> +		ranges;
> >> +
> >> +		ramoops@7fb6a000 {
> >> +			compatible = "ramoops";
> >> +			reg = <0x7fb6a000 0x100000>;
> >> +			ecc-size	= <16>;
> >> +			record-size	= <0x00020000>;
> >> +			console-size	= <0x00020000>;
> >> +			ftrace-size	= <0x00020000>;
> >> +			pmsg-size	= <0x00020000>;
> >> +		};
> >> +	};
> >> +
> > 
> > I'm a bit skeptical about this one.
> 
> I can understand your concern.
> 
> > First, there's nothing specific to the bananapi,
> 
> It is specific to memory size, bootloader used for the board and in some
> cases security co processor. Any thing else I forgot?

Ah, right, you end up depending on the memory size...

> > but every one will want to have different sizes here, or different
> > parameters.
> 
> I don't thing every one need different configurations. I assume current
> parameters are good enough for most users. But if users need some thing
> different they should be overwritten by kernel boot args.

We're basically stuck between two constraints that are opposing: the
users that don't care about ramoops want to use all the RAM they can
and the users that care about ramoops will probably like to have quite
a big buffer to be able to have as much backlog as possible in case of
a crash.

And then, we'd need to define how much exactly "big enough" is, which
is probably going to be subject to debate across the latter population
:)

> May be, DTS should provide some generic/ramoops area and let
> distributions decide how to use it? For example compile kernel with
> some defaults for ramoops.

That would be difficult to achieve, since you don't know where the end
of the RAM is from one board to another (and even between two variants
of the same board sold in different memory size variants).

> > Since using the kernel parameters is also an option, I'd rather
> > document how to do that with the parameters.
> 
> Here are my arguments to do this in DTS:
>
> - some issues are hardly reproducible and it is good to catch it as soon
> as possible. So it should be enabled before first problem will happen.

You'll probably need some extra step anyway though, for example to
load the module.

> - my previous experience with pstore was a bit disappointing, it didn't
> worked on SMP system correctly. So the test coverage seems to be
> minimal. Probably because it is not enabled by default.

That's not really a good argument to enable it though :)

> - at least barebox can extract some postmortem information from not
> booting system. Bootloader and Kernel need to use same parameters,
> devicetree seems to be perfect here.

That could be patched in the DT from bootloaders that are able to do
that then? That would even work better, since they know what the
memory size is.

> - distributions should be able to use this per default as soon as this
> option is available.

This is the issue with defaults, no one agree with them. I can
probably give you a good number of people that would not want to have
that memory at waste. And given the number of current users (3), the
default seems to be of not enabling it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]