| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <55D5BB56.1030405@samsung.com>
Date: Thu, 20 Aug 2015 17:04:46 +0530
From: Pankaj Dubey <pankaj.dubey@...sung.com>
To: Krzysztof Kozlowski <k.kozlowski@...sung.com>,
linux-arm-kernel@...ts.infradead.org,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Cc: thomas.ab@...sung.com, kgene@...nel.org, linux@....linux.org.uk,
heiko@...ech.de
Subject: Re: [PATCH 1/5] drivers: soc: add support for exynos SROM driver
Hi Krzysztof,
Sorry for delay in reply, as I got busy in some other official
assignments and could not take this series further at that time.
On Wednesday 27 May 2015 05:22 PM, Krzysztof Kozlowski wrote:
> W dniu 29.04.2015 o 17:38, Pankaj Dubey pisze:
>> This patch adds Exynos SROM controller driver which will handle
>> save restore of SROM registers during S2R.
>>
>> Change-Id: Iaddaaebc1d7090c9889e948e68e886519562c43c
>
> Please remove it.
Will do it.
>
>> Signed-off-by: Pankaj Dubey <pankaj.dubey@...sung.com>
>> ---
>> drivers/soc/Kconfig | 1 +
>> drivers/soc/Makefile | 1 +
>> drivers/soc/samsung/Kconfig | 14 ++++
>> drivers/soc/samsung/Makefile | 1 +
>> drivers/soc/samsung/exynos-srom.c | 142 ++++++++++++++++++++++++++++++++++++++
>> drivers/soc/samsung/exynos-srom.h | 51 ++++++++++++++
>> 6 files changed, 210 insertions(+)
>> create mode 100644 drivers/soc/samsung/Kconfig
>> create mode 100644 drivers/soc/samsung/Makefile
>> create mode 100644 drivers/soc/samsung/exynos-srom.c
>> create mode 100644 drivers/soc/samsung/exynos-srom.h
>>
>> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
>> index 76d6bd4..c3abfbe 100644
>> --- a/drivers/soc/Kconfig
>> +++ b/drivers/soc/Kconfig
>> @@ -1,6 +1,7 @@
>> menu "SOC (System On Chip) specific Drivers"
>>
>> source "drivers/soc/qcom/Kconfig"
>> +source "drivers/soc/samsung/Kconfig"
>> source "drivers/soc/ti/Kconfig"
>> source "drivers/soc/versatile/Kconfig"
>>
>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
>> index 063113d..620366f 100644
>> --- a/drivers/soc/Makefile
>> +++ b/drivers/soc/Makefile
>> @@ -3,6 +3,7 @@
>> #
>>
>> obj-$(CONFIG_ARCH_QCOM) += qcom/
>> +obj-$(CONFIG_SOC_SAMSUNG) += samsung/
>> obj-$(CONFIG_ARCH_TEGRA) += tegra/
>> obj-$(CONFIG_SOC_TI) += ti/
>> obj-$(CONFIG_PLAT_VERSATILE) += versatile/
>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>> new file mode 100644
>> index 0000000..b6fa4e6
>> --- /dev/null
>> +++ b/drivers/soc/samsung/Kconfig
>> @@ -0,0 +1,14 @@
>> +#
>> +# SAMSUNG SoC drivers
>> +#
>> +menu "Samsung SOC driver support"
>> +
>> +config SOC_SAMSUNG
>> + bool
>
> Any reason for not using menuconfig?
>
For one of my Exynos PMU patchset [1] this suggestion came from Russel
King, not to use user-visible sysmbol if not required.
[1]:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/304451.html
>> +
>> +config EXYNOS_SROM
>> + bool
>> + depends on ARM && ARCH_EXYNOS
>> + select SOC_BUS
>
> Why we need to select SOC_BUS?
>
We do not need it, will modify.
>> +
>> +endmenu
>> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
>> new file mode 100644
>> index 0000000..9c554d5
>> --- /dev/null
>> +++ b/drivers/soc/samsung/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_EXYNOS_SROM) += exynos-srom.o
>> diff --git a/drivers/soc/samsung/exynos-srom.c b/drivers/soc/samsung/exynos-srom.c
>> new file mode 100644
>> index 0000000..8aae762
>> --- /dev/null
>> +++ b/drivers/soc/samsung/exynos-srom.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com/
>> + *
>> + * EXYNOS - SROM Controller support
>> + * Author: Pankaj Dubey <pankaj.dubey@...sung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include "exynos-srom.h"
>> +
>> +static void __iomem *exynos_srom_base;
>> +
>> +static unsigned long exynos_srom_offsets[] = {
>
> static const
>
>> + /* SROM side */
>> + S5P_SROM_BW,
>> + S5P_SROM_BC0,
>> + S5P_SROM_BC1,
>> + S5P_SROM_BC2,
>> + S5P_SROM_BC3,
>> +};
>> +
>> +/**
>> + * struct exynos_srom_reg_dump: register dump of SROM Controller registers.
>> + * @offset: srom register offset from the controller base address.
>> + * @value: the value to be register at offset.
>
> Maybe:
> @value: the value of register under the offset
>
OK.
>> + */
>> +struct exynos_srom_reg_dump {
>> + u32 offset;
>> + u32 value;
>> +};
>> +
>> +static struct exynos_srom_reg_dump *exynos_srom_regs;
>> +
>> +static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump(
>> + const unsigned long *rdump,
>> + unsigned long nr_rdump)
>> +{
>> + struct exynos_srom_reg_dump *rd;
>> + unsigned int i;
>> +
>> + rd = kcalloc(nr_rdump, sizeof(*rd), GFP_KERNEL);
>> + if (!rd)
>> + return NULL;
>> +
>> + for (i = 0; i < nr_rdump; ++i)
>> + rd[i].offset = rdump[i];
>> +
>> + return rd;
>> +}
>> +
>> +static void exynos_srom_save(void __iomem *base,
>> + struct exynos_srom_reg_dump *rd,
>> + unsigned int num_regs)
>> +{
>> + for (; num_regs > 0; --num_regs, ++rd)
>> + rd->value = readl(base + rd->offset);
>> +
>> +}
>> +
>> +static void exynos_srom_restore(void __iomem *base,
>> + const struct exynos_srom_reg_dump *rd,
>> + unsigned int num_regs)
>> +{
>> + for (; num_regs > 0; --num_regs, ++rd)
>> + writel(rd->value, base + rd->offset);
>> +
>> +}
>> +
>> +static const struct of_device_id of_exynos_srom_ids[] = {
>> + {
>> + .compatible = "samsung,exynos-srom",
>> + },
>> + {},
>> +};
>> +
>> +static int exynos_srom_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np;
>> + struct device *dev = &pdev->dev;
>> +
>> + np = dev->of_node;
>> + exynos_srom_base = of_iomap(np, 0);
>> +
>> + if (!exynos_srom_base)
>> + return PTR_ERR(exynos_srom_base);
>
> This looks wrong. If this is NULL, do not convert it to ERR. What will
> be the value of PTR_ERR(NULL)?
>
Ok will handle it properly.
>> +
>> + exynos_srom_regs = exynos_srom_alloc_reg_dump(exynos_srom_offsets,
>> + sizeof(exynos_srom_offsets));
>
> if (NULL) then what? Have you tested the error paths in the probe?
>
I have not tested this for error path, will take care.
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int exynos_srom_suspend(struct device *dev)
>> +{
>> + exynos_srom_save(exynos_srom_base, exynos_srom_regs,
>> + ARRAY_SIZE(exynos_srom_offsets));
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_srom_resume(struct device *dev)
>> +{
>> + exynos_srom_restore(exynos_srom_base, exynos_srom_regs,
>> + ARRAY_SIZE(exynos_srom_offsets));
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops exynos_srom_dev_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(exynos_srom_suspend, exynos_srom_resume)
>> +};
>> +
>> +#define DEV_PM_OPS (&exynos_srom_dev_pm_ops)
>> +#else
>> +#define DEV_PM_OPS NULL
>> +#endif /* CONFIG_PM_SLEEP */
>
> That look like an old code. Please use SIMPLE_DEV_PM_OPS unless there is
> a reason not to.
>
Thanks for suggestion, will update accordingly.
>> +
>> +static struct platform_driver exynos_srom_driver = {
>> + .probe = exynos_srom_probe,
>> + .driver = {
>> + .name = "exynos-srom",
>> + .of_match_table = of_exynos_srom_ids,
>> + .pm = DEV_PM_OPS,
>> + },
>> +};
>> +
>> +static int __init exynos_srom_init(void)
>> +{
>> + return platform_driver_register(&exynos_srom_driver);
>> +}
>> +device_initcall(exynos_srom_init);
>> diff --git a/drivers/soc/samsung/exynos-srom.h b/drivers/soc/samsung/exynos-srom.h
>> new file mode 100644
>> index 0000000..e6ee438
>> --- /dev/null
>> +++ b/drivers/soc/samsung/exynos-srom.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * Exynos SROMC register definitions
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#ifndef __SAMSUNG_REGS_SROM_H
>> +#define __SAMSUNG_REGS_SROM_H __FILE__
>
> The double-inclusion protection looks different than file path and name.
> Please make it similar.
>
OK.
>> +#define S5P_SROMREG(x) (x)
>> +
>> +#define S5P_SROM_BW S5P_SROMREG(0x0)
>> +#define S5P_SROM_BC0 S5P_SROMREG(0x4)
>> +#define S5P_SROM_BC1 S5P_SROMREG(0x8)
>> +#define S5P_SROM_BC2 S5P_SROMREG(0xc)
>> +#define S5P_SROM_BC3 S5P_SROMREG(0x10)
>> +#define S5P_SROM_BC4 S5P_SROMREG(0x14)
>> +#define S5P_SROM_BC5 S5P_SROMREG(0x18)
>> +
>> +/* one register BW holds 4 x 4-bit packed settings for NCS0 - NCS3 */
>> +
>> +#define S5P_SROM_BW__DATAWIDTH__SHIFT 0
>> +#define S5P_SROM_BW__ADDRMODE__SHIFT 1
>> +#define S5P_SROM_BW__WAITENABLE__SHIFT 2
>> +#define S5P_SROM_BW__BYTEENABLE__SHIFT 3
>> +
>> +#define S5P_SROM_BW__CS_MASK 0xf
>> +
>> +#define S5P_SROM_BW__NCS0__SHIFT 0
>> +#define S5P_SROM_BW__NCS1__SHIFT 4
>> +#define S5P_SROM_BW__NCS2__SHIFT 8
>> +#define S5P_SROM_BW__NCS3__SHIFT 12
>> +#define S5P_SROM_BW__NCS4__SHIFT 16
>> +#define S5P_SROM_BW__NCS5__SHIFT 20
>> +
>> +/* applies to same to BCS0 - BCS3 */
>> +
>> +#define S5P_SROM_BCX__PMC__SHIFT 0
>> +#define S5P_SROM_BCX__TACP__SHIFT 4
>> +#define S5P_SROM_BCX__TCAH__SHIFT 8
>> +#define S5P_SROM_BCX__TCOH__SHIFT 12
>> +#define S5P_SROM_BCX__TACC__SHIFT 16
>> +#define S5P_SROM_BCX__TCOS__SHIFT 24
>> +#define S5P_SROM_BCX__TACS__SHIFT 28
>
> s/S5P/EXYNOS/
>
OK.
Thanks,
Pankaj Dubey
>> +
>> +#endif /* __SAMSUNG_REGS_SROM_H */
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists