Re: [PATCH] platform/chrome: Add pstore platform_device

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Mon, Nov 25, 2013 at 11:37:06AM -0800, Olof Johansson wrote:
> Add the ramoops pstore device so that we get logs of panics across reboots.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
> 
>  drivers/platform/chrome/Kconfig           |  14 +++++
>  drivers/platform/chrome/Makefile          |   1 +
>  drivers/platform/chrome/chromeos_pstore.c | 101 ++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
>  create mode 100644 drivers/platform/chrome/chromeos_pstore.c
> 
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index b13303e75a34..06c53c8132ad 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -25,4 +25,18 @@ config CHROMEOS_LAPTOP
>  	  If you have a supported Chromebook, choose Y or M here.
>  	  The module will be called chromeos_laptop.
>  
> +config CHROMEOS_PSTORE
> +	tristate "Chrome OS pstore support"
> +	---help---
> +	  This module instantiates the persistent storage on x86 ChromeOS
> +	  devices. It can be used to store away console logs and crash
> +	  information across reboots.
> +
> +	  The range of memory used is 0xf00000-0x1000000, tradionally the
> +	  memory used to back VGA controller memory.
> +
> +	  If you have a supported Chromebook, choose Y or M here.
> +	  The module will be called chromeos_pstore.
> +
> +
>  endif # CHROMEOS_PLATFORMS
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 015e9195e226..2b860ca7450f 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -1,2 +1,3 @@
>  
>  obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
> +obj-$(CONFIG_CHROMEOS_PSTORE)	+= chromeos_pstore.o
> diff --git a/drivers/platform/chrome/chromeos_pstore.c b/drivers/platform/chrome/chromeos_pstore.c
> new file mode 100644
> index 000000000000..e0e0e65cf442
> --- /dev/null
> +++ b/drivers/platform/chrome/chromeos_pstore.c
> @@ -0,0 +1,101 @@
> +/*
> + *  chromeos_pstore.c - Driver to instantiate Chromebook ramoops device
> + *
> + *  Copyright (C) 2013 Google, Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/dmi.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pstore_ram.h>
> +
> +static struct dmi_system_id chromeos_pstore_dmi_table[] __initdata = {
> +	{
> +		/*
> +		 * Today all Chromebooks/boxes ship with GOOGLE as vendor and
> +		 * coreboot as bios vendor. No other systems with this
> +		 * combination are known to date.
> +		 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		},
> +	},
> +	{
> +		/*
> +		 * The first Samsung Chromebox and Chromebook Series 5 550 use
> +		 * coreboot but with Samsung as the system vendor.
> +		 */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG"),
> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
> +		},
> +	},
> +	{
> +		/* x86-alex, the first Samsung Chromebook. */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Alex"),
> +		},
> +	},
> +	{
> +		/* x86-mario, the Cr-48 pilot device from Google. */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "IEC"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Mario"),
> +		},
> +	},
> +	{
> +		/* x86-zgb, the first Acer Chromebook. */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ACER"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "ZGB"),
> +		},
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dmi, chromeos_pstore_dmi_table);

if you have this ...

> +/*
> + * On x86 chromebooks/boxes, the firmware will keep the legacy VGA memory
> + * range untouched across reboots, so we use that to store our pstore
> + * contents for panic logs, etc.
> + */
> +static struct ramoops_platform_data chromeos_ramoops_data = {
> +	.mem_size	= 0x100000,
> +	.mem_address	= 0xf00000,
> +	.record_size	= 0x20000,
> +	.console_size	= 0x20000,
> +	.ftrace_size	= 0x20000,
> +	.dump_oops	= 1,
> +};
> +
> +static struct platform_device chromeos_ramoops = {
> +	.name = "ramoops",
> +	.dev = {
> +		.platform_data = &chromeos_ramoops_data,
> +	},
> +};
> +
> +static int __init chromeos_pstore_init(void)
> +{
> +	if (dmi_check_system(chromeos_pstore_dmi_table))

is this check really necessary ? I would assume that your probe would
only be called if the device matches the dmi MODULE_DEVICE_TABLE().

Except that you don't have a probe function which is quite odd. Also,
there's nothing using chromeos_ramoops_data, how does this work ?

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]