[PATCH v3 2/2] regulator: Add support for MAX77686.

[PATCH v3 2/2] regulator: Add support for MAX77686.

[yadi.brar01]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

Add support for PMIC/regulator portion of MAX77686 multifunction device.
MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
which supports setting and getting the voltage of a regulator with I2C
interface.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/regulator/Kconfig    |    9 +
 drivers/regulator/Makefile   |    1 +
 drivers/regulator/max77686.c |  387 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 397 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/max77686.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c86b886..e8f9417 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -195,6 +195,15 @@ config REGULATOR_MAX8998
 	  via I2C bus. The provided regulator is suitable for S3C6410
 	  and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
 
+config REGULATOR_MAX77686
+	tristate "Maxim 77686 regulator"
+	depends on MFD_MAX77686
+	help
+	  This driver controls a Maxim 77686 voltage regulator via I2C
+	  bus. The provided regulator is suitable for Exynos5 chips to
+	  control VDD_ARM and VDD_INT voltages.It supports LDOs[1-26]
+	  and BUCKs[1-9].
+
 config REGULATOR_PCAP
 	tristate "Motorola PCAP2 regulator driver"
 	depends on EZX_PCAP
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 977fd46..d854453 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
 obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
 obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
+obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
new file mode 100644
index 0000000..98dbd50
--- /dev/null
+++ b/drivers/regulator/max77686.c
@@ -0,0 +1,387 @@
+/*
+ * max77686.c - Regulator driver for the Maxim 77686
+ *
+ * Copyright (C) 2012 Samsung Electronics Co. Ltd.
+ * Chiwoong Byun <woong.byun@samsung.com>
+ * Yadwinder Singh Brar <yadi.brar@samsung.com>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * This driver is based on max8997.c
+ */
+
+#include <linux/module.h>
+#include <linux/bug.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/mfd/max77686.h>
+#include <linux/mfd/max77686-private.h>
+
+struct max77686_data {
+	struct device *dev;
+	struct max77686_dev *iodev;
+	int num_regulators;
+	struct regulator_dev **rdev;
+	int ramp_delay;		/* index of ramp_delay */
+
+	/*GPIO-DVS feature is not enabled with the
+	 *current version of MAX77686 driver.*/
+};
+
+static int max77686_voltage_dvs_buck_time_sel(struct regulator_dev *rdev,
+					unsigned int old_sel,
+					unsigned int new_sel)
+{
+	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	int ramp[] = {13, 27, 55, 100};	/* ramp_rate in mV/uS */
+
+	return DIV_ROUND_UP(rdev->desc->uV_step *
+			    abs(new_sel - old_sel),
+			    ramp[max77686->ramp_delay]);
+}
+
+static int max77686_voltage_time_sel(struct regulator_dev *rdev,
+					unsigned int old_sel,
+					unsigned int new_sel)
+{
+	return DIV_ROUND_UP(rdev->desc->uV_step *
+			    abs(new_sel - old_sel),
+			    100);
+}
+
+static struct regulator_ops max77686_ops = {
+	.map_voltage		= regulator_map_voltage_linear,
+	.list_voltage		= regulator_list_voltage_linear,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= max77686_voltage_time_sel,
+};
+
+static struct regulator_ops max77686_buck_ops = {
+	.map_voltage		= regulator_map_voltage_linear,
+	.list_voltage		= regulator_list_voltage_linear,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= max77686_voltage_dvs_buck_time_sel,
+};
+
+#define regulator_desc_ldo(num)		{				\
+	.name		= "LDO"#num,					\
+	.id		= MAX77686_LDO##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 800000,					\
+	.uV_step	= 50000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.enable_mask	= 0x0c,						\
+}
+#define regulator_desc_ldo_low_vol(num)	{				\
+	.name		= "LDO"#num,					\
+	.id		= MAX77686_LDO##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 800000,					\
+	.uV_step	= 25000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.enable_mask	= 0x0c,						\
+}
+#define regulator_desc_buck(num)	{				\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 750000,					\
+	.uV_step	= 50000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_BUCK5OUT + (num - 5) * 2,	\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_BUCK5CTRL + (num - 5) * 2,	\
+	.enable_mask	= 0x03,						\
+}
+#define regulator_desc_buck1(num)	{				\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 750000,					\
+	.uV_step	= 50000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_BUCK1OUT,			\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_BUCK1CTRL,			\
+	.enable_mask	= 0x03,						\
+}
+#define regulator_desc_buck_dvs(num)	{				\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_buck_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 600000,					\
+	.uV_step	= 12500,					\
+	.n_voltages	= 256,						\
+	.vsel_reg	= MAX77686_REG_BUCK2DVS1 + (num - 2) * 10,	\
+	.vsel_mask	= 0xff,						\
+	.enable_reg	= MAX77686_REG_BUCK2CTRL1 + (num - 2) * 10,	\
+	.enable_mask	= 0x30,						\
+}
+
+static struct regulator_desc regulators[] = {
+	regulator_desc_ldo_low_vol(1),
+	regulator_desc_ldo_low_vol(2),
+	regulator_desc_ldo(3),
+	regulator_desc_ldo(4),
+	regulator_desc_ldo(5),
+	regulator_desc_ldo_low_vol(6),
+	regulator_desc_ldo_low_vol(7),
+	regulator_desc_ldo_low_vol(8),
+	regulator_desc_ldo(9),
+	regulator_desc_ldo(10),
+	regulator_desc_ldo(11),
+	regulator_desc_ldo(12),
+	regulator_desc_ldo(13),
+	regulator_desc_ldo(14),
+	regulator_desc_ldo(15),
+	regulator_desc_ldo(16),
+	regulator_desc_ldo(17),
+	regulator_desc_ldo(18),
+	regulator_desc_ldo(19),
+	regulator_desc_ldo(20),
+	regulator_desc_ldo(21),
+	regulator_desc_ldo(22),
+	regulator_desc_ldo(23),
+	regulator_desc_ldo(24),
+	regulator_desc_ldo(25),
+	regulator_desc_ldo(26),
+	regulator_desc_buck1(1),
+	regulator_desc_buck_dvs(2),
+	regulator_desc_buck_dvs(3),
+	regulator_desc_buck_dvs(4),
+	regulator_desc_buck(5),
+	regulator_desc_buck(6),
+	regulator_desc_buck(7),
+	regulator_desc_buck(8),
+	regulator_desc_buck(9),
+};
+
+#ifdef CONFIG_OF
+static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
+					struct max77686_platform_data *pdata)
+{
+	struct device_node *pmic_np, *regulators_np;
+	struct of_regulator_match *rdata;
+	unsigned int i, ret;
+
+	pmic_np = iodev->dev->of_node;
+	if (!pmic_np) {
+		dev_err(iodev->dev, "could not find pmic sub-node\n");
+		return -ENODEV;
+	}
+
+	regulators_np = of_find_node_by_name(pmic_np, "voltage-regulators");
+	if (!regulators_np) {
+		dev_err(iodev->dev, "could not find regulators sub-node\n");
+		return -EINVAL;
+	}
+
+	/* count the number of regulators to be supported in pmic */
+	pdata->num_regulators = ARRAY_SIZE(regulators);
+
+	rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) *
+				(pdata->num_regulators), GFP_KERNEL);
+	if (!rdata) {
+		dev_err(iodev->dev,
+			"could not allocate memory for regulator data\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < pdata->num_regulators; i++)
+		rdata[i].name = regulators[i].name;
+
+	ret = of_regulator_match(iodev->dev, regulators_np, rdata,
+				pdata->num_regulators);
+
+	if (ret < 0)
+		dev_err(iodev->dev, "Parsing DT for regulators failed\n");
+	else
+		dev_info(iodev->dev, "regulators found in device tree : %d\n"
+			, ret);
+
+	pdata->regulators = rdata;
+
+	if (of_property_read_u32(pmic_np, "max77686,buck_ramp_delay", &i))
+		pdata->ramp_delay = i & 0xff;
+
+	return 0;
+}
+#else
+static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
+					struct max77686_platform_data *pdata)
+{
+	return 0;
+}
+#endif	/* CONFIG_OF */
+
+static __devinit int max77686_pmic_probe(struct platform_device *pdev)
+{
+	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+	struct max77686_platform_data *pdata = iodev->pdata;
+	struct regulator_dev **rdev;
+	struct max77686_data *max77686;
+	struct i2c_client *i2c = iodev->i2c;
+	struct regulator_config config = { };
+	int i, ret, size;
+
+	if (iodev->dev->of_node) {
+		ret = max77686_pmic_dt_parse_pdata(iodev, pdata);
+		if (ret)
+			return ret;
+	} else {			/* pdata from machine-setup file */
+		if (!pdata) {
+			dev_err(&pdev->dev, "platform data not found\n");
+			return -ENODEV;
+		} else {
+			if (pdata->num_regulators != ARRAY_SIZE(regulators)) {
+				dev_err(&pdev->dev,
+					"incomplete regulator list\n");
+				return -ENODEV;
+			}
+		}
+	}
+
+	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
+				GFP_KERNEL);
+	if (!max77686)
+		return -ENOMEM;
+
+	size = sizeof(struct regulator_dev *) * pdata->num_regulators;
+	max77686->rdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!max77686->rdev) {
+		kfree(max77686);
+		return -ENOMEM;
+	}
+
+	rdev = max77686->rdev;
+
+	max77686->dev = &pdev->dev;
+	max77686->iodev = iodev;
+	max77686->num_regulators = pdata->num_regulators;
+
+	if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV ||
+		pdata->ramp_delay > MAX77686_RAMP_RATE_100MV)
+		pdata->ramp_delay = MAX77686_RAMP_RATE_27MV;	/* default */
+
+	max77686->ramp_delay = pdata->ramp_delay - 1;
+	max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
+		max77686->ramp_delay << 6, RAMP_MASK);
+	max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
+		max77686->ramp_delay << 6, RAMP_MASK);
+	max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
+		max77686->ramp_delay << 6, RAMP_MASK);
+
+	platform_set_drvdata(pdev, max77686);
+
+	for (i = 0; i < pdata->num_regulators; i++) {
+		config.dev = max77686->dev;
+		config.init_data = pdata->regulators[i].init_data;
+		config.driver_data = max77686;
+		config.regmap = iodev->regmap;
+
+		rdev[i] = regulator_register(&regulators[i], &config);
+		if (IS_ERR(rdev[i])) {
+			ret = PTR_ERR(rdev[i]);
+			dev_err(max77686->dev,
+				"regulator init failed for id : %d\n", i);
+			rdev[i] = NULL;
+			goto err;
+		}
+	}
+
+	return 0;
+ err:
+	for (i = 0; i < max77686->num_regulators; i++)
+		if (rdev[i])
+			regulator_unregister(rdev[i]);
+
+	return ret;
+}
+
+static int __devexit max77686_pmic_remove(struct platform_device *pdev)
+{
+	struct max77686_data *max77686 = platform_get_drvdata(pdev);
+	struct regulator_dev **rdev = max77686->rdev;
+	int i;
+
+	for (i = 0; i < max77686->num_regulators; i++)
+		if (rdev[i])
+			regulator_unregister(rdev[i]);
+
+	return 0;
+}
+
+static const struct platform_device_id max77686_pmic_id[] = {
+	{"max77686-pmic", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(platform, max77686_pmic_id);
+
+static struct platform_driver max77686_pmic_driver = {
+	.driver = {
+		   .name = "max77686-pmic",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = max77686_pmic_probe,
+	.remove = __devexit_p(max77686_pmic_remove),
+	.id_table = max77686_pmic_id,
+};
+
+static int __init max77686_pmic_init(void)
+{
+	return platform_driver_register(&max77686_pmic_driver);
+}
+subsys_initcall(max77686_pmic_init);
+
+static void __exit max77686_pmic_cleanup(void)
+{
+	platform_driver_unregister(&max77686_pmic_driver);
+}
+module_exit(max77686_pmic_cleanup);
+
+MODULE_DESCRIPTION("MAXIM 77686 Regulator Driver");
+MODULE_AUTHOR("Chiwoong Byun <woong.byun@samsung.com>");
+MODULE_AUTHOR("Yadwinder Singh Brar <yadi.brar@samsung.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

Hi, Yadwinder,
As you know, both of us, recently, had competition for one driver
whether you intend or not. And now, i think it is time to stop this and
to find appropriate goal. From now on, i won't update this driver no
more. I recommend you to review my patch and apply feature that you can
apply. And also check comments that i wrote below.

On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:

> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> Add support for PMIC/regulator portion of MAX77686 multifunction device.
> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
> which supports setting and getting the voltage of a regulator with I2C
> interface.
> 
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/regulator/Kconfig    |    9 +
>  drivers/regulator/Makefile   |    1 +
>  drivers/regulator/max77686.c |  387 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 397 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/max77686.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index c86b886..e8f9417 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -195,6 +195,15 @@ config REGULATOR_MAX8998
>  	  via I2C bus. The provided regulator is suitable for S3C6410
>  	  and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
>  
> +config REGULATOR_MAX77686
> +	tristate "Maxim 77686 regulator"
> +	depends on MFD_MAX77686
> +	help
> +	  This driver controls a Maxim 77686 voltage regulator via I2C
> +	  bus. The provided regulator is suitable for Exynos5 chips to
> +	  control VDD_ARM and VDD_INT voltages.It supports LDOs[1-26]
> +	  and BUCKs[1-9].
> +
>  config REGULATOR_PCAP
>  	tristate "Motorola PCAP2 regulator driver"
>  	depends on EZX_PCAP
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 977fd46..d854453 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
>  obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
>  obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
>  obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
> +obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
>  obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> new file mode 100644
> index 0000000..98dbd50
> --- /dev/null
> +++ b/drivers/regulator/max77686.c
> @@ -0,0 +1,387 @@
> +/*
> + * max77686.c - Regulator driver for the Maxim 77686
> + *
> + * Copyright (C) 2012 Samsung Electronics Co. Ltd.
> + * Chiwoong Byun <woong.byun@samsung.com>
> + * Yadwinder Singh Brar <yadi.brar@samsung.com>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + * This driver is based on max8997.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/bug.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/mfd/max77686.h>
> +#include <linux/mfd/max77686-private.h>
> +
> +struct max77686_data {
> +	struct device *dev;
> +	struct max77686_dev *iodev;
> +	int num_regulators;
> +	struct regulator_dev **rdev;
> +	int ramp_delay;		/* index of ramp_delay */
> +
> +	/*GPIO-DVS feature is not enabled with the
> +	 *current version of MAX77686 driver.*/
> +};
> +
> +static int max77686_voltage_dvs_buck_time_sel(struct regulator_dev *rdev,
> +					unsigned int old_sel,
> +					unsigned int new_sel)
> +{
> +	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
> +	int ramp[] = {13, 27, 55, 100};	/* ramp_rate in mV/uS */
> +
> +	return DIV_ROUND_UP(rdev->desc->uV_step *
> +			    abs(new_sel - old_sel),
> +			    ramp[max77686->ramp_delay]);
> +}
> +
> +static int max77686_voltage_time_sel(struct regulator_dev *rdev,
> +					unsigned int old_sel,
> +					unsigned int new_sel)
> +{
> +	return DIV_ROUND_UP(rdev->desc->uV_step *
> +			    abs(new_sel - old_sel),
> +			    100);
> +}
> +


Does LDO also need waiting for voltage change? I afraid it's not.

> +static struct regulator_ops max77686_ops = {
> +	.map_voltage		= regulator_map_voltage_linear,
> +	.list_voltage		= regulator_list_voltage_linear,
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.enable			= regulator_enable_regmap,
> +	.disable		= regulator_disable_regmap,
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.set_voltage_time_sel	= max77686_voltage_time_sel,
> +};
> +
> +static struct regulator_ops max77686_buck_ops = {
> +	.map_voltage		= regulator_map_voltage_linear,
> +	.list_voltage		= regulator_list_voltage_linear,
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.enable			= regulator_enable_regmap,
> +	.disable		= regulator_disable_regmap,
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.set_voltage_time_sel	= max77686_voltage_dvs_buck_time_sel,
> +};
> +
> +#define regulator_desc_ldo(num)		{				\
> +	.name		= "LDO"#num,					\
> +	.id		= MAX77686_LDO##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 800000,					\
> +	.uV_step	= 50000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.enable_mask	= 0x0c,						\
> +}
> +#define regulator_desc_ldo_low_vol(num)	{				\
> +	.name		= "LDO"#num,					\
> +	.id		= MAX77686_LDO##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 800000,					\
> +	.uV_step	= 25000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.enable_mask	= 0x0c,						\
> +}
> +#define regulator_desc_buck(num)	{				\
> +	.name		= "BUCK"#num,					\
> +	.id		= MAX77686_BUCK##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 750000,					\
> +	.uV_step	= 50000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_BUCK5OUT + (num - 5) * 2,	\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_BUCK5CTRL + (num - 5) * 2,	\
> +	.enable_mask	= 0x03,						\
> +}
> +#define regulator_desc_buck1(num)	{				\
> +	.name		= "BUCK"#num,					\
> +	.id		= MAX77686_BUCK##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 750000,					\
> +	.uV_step	= 50000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_BUCK1OUT,			\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_BUCK1CTRL,			\
> +	.enable_mask	= 0x03,						\
> +}
> +#define regulator_desc_buck_dvs(num)	{				\
> +	.name		= "BUCK"#num,					\
> +	.id		= MAX77686_BUCK##num,				\
> +	.ops		= &max77686_buck_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 600000,					\
> +	.uV_step	= 12500,					\
> +	.n_voltages	= 256,						\
> +	.vsel_reg	= MAX77686_REG_BUCK2DVS1 + (num - 2) * 10,	\
> +	.vsel_mask	= 0xff,						\
> +	.enable_reg	= MAX77686_REG_BUCK2CTRL1 + (num - 2) * 10,	\
> +	.enable_mask	= 0x30,						\
> +}
> +
> +static struct regulator_desc regulators[] = {
> +	regulator_desc_ldo_low_vol(1),
> +	regulator_desc_ldo_low_vol(2),
> +	regulator_desc_ldo(3),
> +	regulator_desc_ldo(4),
> +	regulator_desc_ldo(5),
> +	regulator_desc_ldo_low_vol(6),
> +	regulator_desc_ldo_low_vol(7),
> +	regulator_desc_ldo_low_vol(8),
> +	regulator_desc_ldo(9),
> +	regulator_desc_ldo(10),
> +	regulator_desc_ldo(11),
> +	regulator_desc_ldo(12),
> +	regulator_desc_ldo(13),
> +	regulator_desc_ldo(14),
> +	regulator_desc_ldo(15),
> +	regulator_desc_ldo(16),
> +	regulator_desc_ldo(17),
> +	regulator_desc_ldo(18),
> +	regulator_desc_ldo(19),
> +	regulator_desc_ldo(20),
> +	regulator_desc_ldo(21),
> +	regulator_desc_ldo(22),
> +	regulator_desc_ldo(23),
> +	regulator_desc_ldo(24),
> +	regulator_desc_ldo(25),
> +	regulator_desc_ldo(26),
> +	regulator_desc_buck1(1),
> +	regulator_desc_buck_dvs(2),
> +	regulator_desc_buck_dvs(3),
> +	regulator_desc_buck_dvs(4),
> +	regulator_desc_buck(5),
> +	regulator_desc_buck(6),
> +	regulator_desc_buck(7),
> +	regulator_desc_buck(8),
> +	regulator_desc_buck(9),
> +};
> +
> +#ifdef CONFIG_OF
> +static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
> +					struct max77686_platform_data *pdata)
> +{
> +	struct device_node *pmic_np, *regulators_np;
> +	struct of_regulator_match *rdata;
> +	unsigned int i, ret;
> +
> +	pmic_np = iodev->dev->of_node;
> +	if (!pmic_np) {
> +		dev_err(iodev->dev, "could not find pmic sub-node\n");
> +		return -ENODEV;
> +	}
> +
> +	regulators_np = of_find_node_by_name(pmic_np, "voltage-regulators");
> +	if (!regulators_np) {
> +		dev_err(iodev->dev, "could not find regulators sub-node\n");
> +		return -EINVAL;
> +	}
> +
> +	/* count the number of regulators to be supported in pmic */
> +	pdata->num_regulators = ARRAY_SIZE(regulators);
> +
> +	rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) *
> +				(pdata->num_regulators), GFP_KERNEL);
> +	if (!rdata) {
> +		dev_err(iodev->dev,
> +			"could not allocate memory for regulator data\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < pdata->num_regulators; i++)
> +		rdata[i].name = regulators[i].name;
> +
> +	ret = of_regulator_match(iodev->dev, regulators_np, rdata,
> +				pdata->num_regulators);
> +
> +	if (ret < 0)
> +		dev_err(iodev->dev, "Parsing DT for regulators failed\n");
> +	else
> +		dev_info(iodev->dev, "regulators found in device tree : %d\n"
> +			, ret);
> +
> +	pdata->regulators = rdata;
> +
> +	if (of_property_read_u32(pmic_np, "max77686,buck_ramp_delay", &i))
> +		pdata->ramp_delay = i & 0xff;
> +
> +	return 0;
> +}
> +#else
> +static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
> +					struct max77686_platform_data *pdata)
> +{
> +	return 0;
> +}
> +#endif	/* CONFIG_OF */
> +
> +static __devinit int max77686_pmic_probe(struct platform_device *pdev)
> +{
> +	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> +	struct max77686_platform_data *pdata = iodev->pdata;
> +	struct regulator_dev **rdev;
> +	struct max77686_data *max77686;
> +	struct i2c_client *i2c = iodev->i2c;
> +	struct regulator_config config = { };
> +	int i, ret, size;
> +
> +	if (iodev->dev->of_node) {
> +		ret = max77686_pmic_dt_parse_pdata(iodev, pdata);
> +		if (ret)
> +			return ret;
> +	} else {			/* pdata from machine-setup file */
> +		if (!pdata) {
> +			dev_err(&pdev->dev, "platform data not found\n");
> +			return -ENODEV;
> +		} else {
> +			if (pdata->num_regulators != ARRAY_SIZE(regulators)) {
> +				dev_err(&pdev->dev,
> +					"incomplete regulator list\n");
> +				return -ENODEV;
> +			}
> +		}
> +	}
> +
> +	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
> +				GFP_KERNEL);
> +	if (!max77686)
> +		return -ENOMEM;
> +
> +	size = sizeof(struct regulator_dev *) * pdata->num_regulators;
> +	max77686->rdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> +	if (!max77686->rdev) {
> +		kfree(max77686);
> +		return -ENOMEM;
> +	}
> +
> +	rdev = max77686->rdev;
> +
> +	max77686->dev = &pdev->dev;
> +	max77686->iodev = iodev;
> +	max77686->num_regulators = pdata->num_regulators;
> +
> +	if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV ||
> +		pdata->ramp_delay > MAX77686_RAMP_RATE_100MV)
> +		pdata->ramp_delay = MAX77686_RAMP_RATE_27MV;	/* default */
> +
> +	max77686->ramp_delay = pdata->ramp_delay - 1;


I think it is better to check pdata->ramp_delay is available.
If pdata doesn't have ramp_delay member it might be error.

> +	max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
> +		max77686->ramp_delay << 6, RAMP_MASK);
> +	max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
> +		max77686->ramp_delay << 6, RAMP_MASK);
> +	max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
> +		max77686->ramp_delay << 6, RAMP_MASK);
> +


Why do you use i2c client still? If you registered regmap you can use
its API. I recommend you to use regmap_update_bits() directly.


> +	platform_set_drvdata(pdev, max77686);
> +
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		config.dev = max77686->dev;
> +		config.init_data = pdata->regulators[i].init_data;
> +		config.driver_data = max77686;
> +		config.regmap = iodev->regmap;
> +
> +		rdev[i] = regulator_register(&regulators[i], &config);
> +		if (IS_ERR(rdev[i])) {
> +			ret = PTR_ERR(rdev[i]);
> +			dev_err(max77686->dev,
> +				"regulator init failed for id : %d\n", i);
> +			rdev[i] = NULL;
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> + err:
> +	for (i = 0; i < max77686->num_regulators; i++)
> +		if (rdev[i])
> +			regulator_unregister(rdev[i]);
> +
> +	return ret;
> +}
> +
> +static int __devexit max77686_pmic_remove(struct platform_device *pdev)
> +{
> +	struct max77686_data *max77686 = platform_get_drvdata(pdev);
> +	struct regulator_dev **rdev = max77686->rdev;
> +	int i;
> +
> +	for (i = 0; i < max77686->num_regulators; i++)
> +		if (rdev[i])
> +			regulator_unregister(rdev[i]);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id max77686_pmic_id[] = {
> +	{"max77686-pmic", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, max77686_pmic_id);
> +
> +static struct platform_driver max77686_pmic_driver = {
> +	.driver = {
> +		   .name = "max77686-pmic",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe = max77686_pmic_probe,
> +	.remove = __devexit_p(max77686_pmic_remove),
> +	.id_table = max77686_pmic_id,
> +};
> +
> +static int __init max77686_pmic_init(void)
> +{
> +	return platform_driver_register(&max77686_pmic_driver);
> +}
> +subsys_initcall(max77686_pmic_init);
> +
> +static void __exit max77686_pmic_cleanup(void)
> +{
> +	platform_driver_unregister(&max77686_pmic_driver);
> +}
> +module_exit(max77686_pmic_cleanup);
> +
> +MODULE_DESCRIPTION("MAXIM 77686 Regulator Driver");
> +MODULE_AUTHOR("Chiwoong Byun <woong.byun@samsung.com>");
> +MODULE_AUTHOR("Yadwinder Singh Brar <yadi.brar@samsung.com>");
> +MODULE_LICENSE("GPL");


MAX77686 has crystal oscillator in it. And original version of this
driver which was written by Chiwoon Byun, registers it as a regulator.
As Mark says, we have to change it to use generic clock API. Where do
you think should we put them into? In my opinion, it is proper that just
leave them in regulator driver because this driver is almost core of
PMIC. I already applied generic API in my local repository but i
couldn't test yet. Because it crashed with SOC's private clock API.
Anyway if you register 32khz clock with generic API ,DEFINE_CLK_GATE()
will help you out which defined in linux/clk-private.h.

Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

(adding Kyungmin Park and Samuel Ortiz)

Hi,

Yes, It happened unintentionally. I didn't know about your patch
before submitting
the initial version of my patches. I agree with you, I will review
your patches and
will try to incorporate extra features from your patches.

On Wed, May 23, 2012 at 7:10 AM,  <jonghwa3.lee@samsung.com> wrote:
> Hi, Yadwinder,
> As you know, both of us, recently, had competition for one driver
> whether you intend or not. And now, i think it is time to stop this and
> to find appropriate goal. From now on, i won't update this driver no
> more. I recommend you to review my patch and apply feature that you can
> apply. And also check comments that i wrote below.
>
> On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:
>
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> Add support for PMIC/regulator portion of MAX77686 multifunction device.
>> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
>> which supports setting and getting the voltage of a regulator with I2C
>> interface.

>> +     return DIV_ROUND_UP(rdev->desc->uV_step *
>> +                         abs(new_sel - old_sel),
>> +                         100);
>> +}
>> +
>
>
> Does LDO also need waiting for voltage change? I afraid it's not.
>

Yes, according to technical reference manual which I have, ramp rate for
LDOs is also 100mV/us.

>> +
>> +     if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV ||
>> +             pdata->ramp_delay > MAX77686_RAMP_RATE_100MV)
>> +             pdata->ramp_delay = MAX77686_RAMP_RATE_27MV;    /* default */

If pdata doesn't have proper ramp_delay, it will get value
MAX77686_RAMP_RATE_27MV.

>> +
>> +     max77686->ramp_delay = pdata->ramp_delay - 1;
>
>
> I think it is better to check pdata->ramp_delay is available.
> If pdata doesn't have ramp_delay member it might be error.
>

Yes, we have taken care of this case above before setting value of
max77686->ramp_delay.

>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>> +             max77686->ramp_delay << 6, RAMP_MASK);
>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>> +             max77686->ramp_delay << 6, RAMP_MASK);
>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>> +             max77686->ramp_delay << 6, RAMP_MASK);
>> +
>
>
> Why do you use i2c client still? If you registered regmap you can use
> its API. I recommend you to use regmap_update_bits() directly.
>
>

Yes, we are using regmap_update_bits().  max77686_update_reg() is just
a wrapper over it.

>> +     platform_set_drvdata(pdev, max77686);
>> +

>
> MAX77686 has crystal oscillator in it. And original version of this
> driver which was written by Chiwoon Byun, registers it as a regulator.
> As Mark says, we have to change it to use generic clock API. Where do
> you think should we put them into? In my opinion, it is proper that just
> leave them in regulator driver because this driver is almost core of
> PMIC. I already applied generic API in my local repository but i
> couldn't test yet. Because it crashed with SOC's private clock API.
> Anyway if you register 32khz clock with generic API ,DEFINE_CLK_GATE()
> will help you out which defined in linux/clk-private.h.
>

Yes, I was also thinking about where to put it. I am not sure whether this
is a proper place to put them. Anyway I will again think about it.

Thanks,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote:

>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>> +
>>
>>
>> Why do you use i2c client still? If you registered regmap you can use
>> its API. I recommend you to use regmap_update_bits() directly.
>>
>>
> 
> Yes, we are using regmap_update_bits().  max77686_update_reg() is just
> a wrapper over it.
> 


Yes, i know what you mean. However it doesn't need max77686_update_reg()
any more since it uses regmap API. Why don't you just pass iodev->regmap
to regmap_update_bits(). It is clear that there is no reason for using
i2c client as a medium. Please check regulator and mfd driver of my
previous patch.


Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

On Wed, May 23, 2012 at 10:10 AM,  <jonghwa3.lee@samsung.com> wrote:
> On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote:
>
>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>> +
>>>
>>>
>>> Why do you use i2c client still? If you registered regmap you can use
>>> its API. I recommend you to use regmap_update_bits() directly.
>>>
>>>
>>
>> Yes, we are using regmap_update_bits().  max77686_update_reg() is just
>> a wrapper over it.
>>
>
>
> Yes, i know what you mean. However it doesn't need max77686_update_reg()
> any more since it uses regmap API. Why don't you just pass iodev->regmap
> to regmap_update_bits(). It is clear that there is no reason for using
> i2c client as a medium. Please check regulator and mfd driver of my
> previous patch.
>

I agree with you we can use directly  regmap API. But I preferred
max77686_update_reg() because its a common practice to use
common read/write API which we define in mfd driver to access
that particular mfd device from other drivers.

Regards,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

On 2012년 05월 23일 14:23, Yadwinder Singh Brar wrote:

> On Wed, May 23, 2012 at 10:10 AM,  <jonghwa3.lee@samsung.com> wrote:
>> On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote:
>>
>>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>>> +
>>>>
>>>>
>>>> Why do you use i2c client still? If you registered regmap you can use
>>>> its API. I recommend you to use regmap_update_bits() directly.
>>>>
>>>>
>>>
>>> Yes, we are using regmap_update_bits().  max77686_update_reg() is just
>>> a wrapper over it.
>>>
>>
>>
>> Yes, i know what you mean. However it doesn't need max77686_update_reg()
>> any more since it uses regmap API. Why don't you just pass iodev->regmap
>> to regmap_update_bits(). It is clear that there is no reason for using
>> i2c client as a medium. Please check regulator and mfd driver of my
>> previous patch.
>>
> 
> I agree with you we can use directly  regmap API. But I preferred
> max77686_update_reg() because its a common practice to use
> common read/write API which we define in mfd driver to access
> that particular mfd device from other drivers.
> 
> Regards,
> Yadwinder.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


I inform you my mfd driver has been confirmed by Samuel Oritz and there
is no mfd private API. This situation looks unusual that we registers
mfd driver and regulator driver separately. But how should we do? For
corporation , i'm asking you to consider my suggestion.

Thanks.


Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Mark Brown]

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

On Wed, May 23, 2012 at 02:33:28PM +0900, jonghwa3.lee@samsung.com wrote:

> I inform you my mfd driver has been confirmed by Samuel Oritz and there
> is no mfd private API. This situation looks unusual that we registers
> mfd driver and regulator driver separately. But how should we do? For
> corporation , i'm asking you to consider my suggestion.

Given all this discussion I'm going to ignore this patch for now and
wait for a repost after you've come to an agreement.

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

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

Hi Jonghwa,

I didn't know about confirmation of your mfd driver by Samuel Oritz.
So please feel free to revise and submit your patch set.
Anyways, I am interested only in getting the driver for max77686 in mainline.

Thanks,
Yadwinder.

On Wed, May 23, 2012 at 3:48 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 23, 2012 at 02:33:28PM +0900, jonghwa3.lee@samsung.com wrote:
>
>> I inform you my mfd driver has been confirmed by Samuel Oritz and there
>> is no mfd private API. This situation looks unusual that we registers
>> mfd driver and regulator driver separately. But how should we do? For
>> corporation , i'm asking you to consider my suggestion.
>
> Given all this discussion I'm going to ignore this patch for now and
> wait for a repost after you've come to an agreement.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

On Wed, May 23, 2012 at 9:46 AM, Yadwinder Singh Brar
<yadi.brar01@gmail.com> wrote:
> (adding Kyungmin Park and Samuel Ortiz)
>
> Hi,
>
> Yes, It happened unintentionally. I didn't know about your patch
> before submitting
> the initial version of my patches. I agree with you, I will review
> your patches and
> will try to incorporate extra features from your patches.
>

Now I have seen your patches for mfd and regulator drivers.
Apparently, it seems that mostly we same features in our patches.
Their is no extra feature to be incorporated form your patches.
Rather I found device tree support is additional in our patches
and mainly their are some differences related to DVS_GPIO and
opmode stuff in our patches:
1- Since we are not implementing and using DVS feature
through GPIOs, so all(incomplete) stuff related to dvs_gpio
is not required currently in our mfd driver presently.
2- Since presently, we are not implementing suspend_enable/disable
callbacks in regulator driver, So we don't need opmode related
stuff now because I think regulators should come up in normal mode
only through .enable callback function.

> On Wed, May 23, 2012 at 7:10 AM,  <jonghwa3.lee@samsung.com> wrote:
>> Hi, Yadwinder,
>> As you know, both of us, recently, had competition for one driver
>> whether you intend or not. And now, i think it is time to stop this and
>> to find appropriate goal. From now on, i won't update this driver no
>> more. I recommend you to review my patch and apply feature that you can
>> apply. And also check comments that i wrote below.
>>

Thanks,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

Hi, again.
On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:


> +static __devinit int max77686_pmic_probe(struct platform_device *pdev)
> +{

> +
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		config.dev = max77686->dev;
> +		config.init_data = pdata->regulators[i].init_data;
> +		config.driver_data = max77686;
> +		config.regmap = iodev->regmap;
> +
> +		rdev[i] = regulator_register(&regulators[i], &config);


I'm sorry that i missed one. You have to register all regulators
unconditionally. Mark brown commented about this to my former patch.

'No, you should unconditionally register all regulators the device
physically has.  This is useful for debug and simplifies the code.'
						- from Mark Brown



Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

Hi,

On Wed, May 23, 2012 at 7:20 AM,  <jonghwa3.lee@samsung.com> wrote:
> Hi, again.
> On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:
>
>
>> +static __devinit int max77686_pmic_probe(struct platform_device *pdev)
>> +{
>
>> +
>> +     for (i = 0; i < pdata->num_regulators; i++) {
>> +             config.dev = max77686->dev;
>> +             config.init_data = pdata->regulators[i].init_data;
>> +             config.driver_data = max77686;
>> +             config.regmap = iodev->regmap;
>> +
>> +             rdev[i] = regulator_register(&regulators[i], &config);
>
>
> I'm sorry that i missed one. You have to register all regulators
> unconditionally. Mark brown commented about this to my former patch.
>
> 'No, you should unconditionally register all regulators the device
> physically has.  This is useful for debug and simplifies the code.'
>                                                - from Mark Brown
>

Yes, we are registering all regulators here.
As pdata->num_regulators will be equal to ARRAY_SIZE(regulators)

Thanks,
Yadwinder.

[PATCH] extcon: Add missing REGMAP_I2C/REGMAP_IRQ dependency on extcon driver

[Chanwoo Choi]

This patch add missing REGMAP_I2C/REGMAP_IRQ dependency on extcon provider
driver to protect build break.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6f2f472..77577db 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -23,6 +23,7 @@ config EXTCON_ADC_JACK
 config EXTCON_ARIZONA
 	tristate "Wolfson Arizona EXTCON support"
 	depends on MFD_ARIZONA && INPUT && SND_SOC
+	select REGMAP_I2C
 	help
 	  Say Y here to enable support for external accessory detection
 	  with Wolfson Arizona devices. These are audio CODECs with
@@ -40,6 +41,7 @@ config EXTCON_MAX14577
 	depends on MFD_MAX14577
 	select IRQ_DOMAIN
 	select REGMAP_I2C
+	select REGMAP_IRQ
 	help
 	  If you say yes here you get support for the MUIC device of
 	  Maxim MAX14577/77836. The MAX14577/77836 MUIC is a USB port accessory
@@ -50,6 +52,7 @@ config EXTCON_MAX77693
 	depends on MFD_MAX77693 && INPUT
 	select IRQ_DOMAIN
 	select REGMAP_I2C
+	select REGMAP_IRQ
 	help
 	  If you say yes here you get support for the MUIC device of
 	  Maxim MAX77693 PMIC. The MAX77693 MUIC is a USB port accessory
@@ -66,6 +69,7 @@ config EXTCON_MAX8997
 config EXTCON_PALMAS
 	tristate "Palmas USB EXTCON support"
 	depends on MFD_PALMAS
+	select REGMAP_IRQ
 	help
 	  Say Y here to enable support for USB peripheral and USB host
 	  detection by palmas usb.
-- 
1.8.0

Re: [PATCH] extcon: Add missing REGMAP_I2C/REGMAP_IRQ dependency on extcon driver

[Krzysztof Kozlowski]

On śro, 2014-07-23 at 10:40 +0900, Chanwoo Choi wrote:
> This patch add missing REGMAP_I2C/REGMAP_IRQ dependency on extcon provider
> driver to protect build break.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/extcon/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 6f2f472..77577db 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -23,6 +23,7 @@ config EXTCON_ADC_JACK
>  config EXTCON_ARIZONA
>  	tristate "Wolfson Arizona EXTCON support"
>  	depends on MFD_ARIZONA && INPUT && SND_SOC
> +	select REGMAP_I2C
>  	help
>  	  Say Y here to enable support for external accessory detection
>  	  with Wolfson Arizona devices. These are audio CODECs with
> @@ -40,6 +41,7 @@ config EXTCON_MAX14577
>  	depends on MFD_MAX14577
>  	select IRQ_DOMAIN
>  	select REGMAP_I2C
> +	select REGMAP_IRQ

Hi,

Is a build break possible here? I think not because this depends on
MFD_MAX14577 which selects both REGMAP_I2C and REGMAP_IRQ.

This change seems not needed then.

>  	help
>  	  If you say yes here you get support for the MUIC device of
>  	  Maxim MAX14577/77836. The MAX14577/77836 MUIC is a USB port accessory
> @@ -50,6 +52,7 @@ config EXTCON_MAX77693
>  	depends on MFD_MAX77693 && INPUT
>  	select IRQ_DOMAIN
>  	select REGMAP_I2C
> +	select REGMAP_IRQ

The same applies here.

>  	help
>  	  If you say yes here you get support for the MUIC device of
>  	  Maxim MAX77693 PMIC. The MAX77693 MUIC is a USB port accessory
> @@ -66,6 +69,7 @@ config EXTCON_MAX8997
>  config EXTCON_PALMAS
>  	tristate "Palmas USB EXTCON support"
>  	depends on MFD_PALMAS
> +	select REGMAP_IRQ

The same applies here.

Best regards,
Krzysztof

>  	help
>  	  Say Y here to enable support for USB peripheral and USB host
>  	  detection by palmas usb.

Re: [PATCH] extcon: Add missing REGMAP_I2C/REGMAP_IRQ dependency on extcon driver

[Charles Keepax]

On Wed, Jul 23, 2014 at 10:40:40AM +0900, Chanwoo Choi wrote:
> This patch add missing REGMAP_I2C/REGMAP_IRQ dependency on extcon provider
> driver to protect build break.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/extcon/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 6f2f472..77577db 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -23,6 +23,7 @@ config EXTCON_ADC_JACK
>  config EXTCON_ARIZONA
>  	tristate "Wolfson Arizona EXTCON support"
>  	depends on MFD_ARIZONA && INPUT && SND_SOC
> +	select REGMAP_I2C
>  	help
>  	  Say Y here to enable support for external accessory detection
>  	  with Wolfson Arizona devices. These are audio CODECs with

Could you be more specific about the config that causes the build
break? This seems problematic as REGMAP_I2C is only a dependancy
if I2C is being used otherwise REGMAP_SPI would be the
dependancy.

Thanks,
Charles

[PATCHv2 0/5] rtc: s3c: Refactoring s3c-rtc driver and support Exynos3250 RTC

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patchset clean up codes to improve readability as following and support
the RTC of Exynos3250 SoC.
- Remove global variables and then use new s3c_rtc structure
- Remove warn message with checking checkpatch script
- Use variant structure according to SoC type instead of legacy enum variable(s3c_cpu_type)

Changes from v1:
- Fix NULL pointer panic of s3c_rtc_settime()
- Check info->base value to protect NULL pointer panic

Chanwoo Choi (5):
  rtc: s3c: Define s3c_rtc structure to remove global variables.
  rtc: s3c: Remove warning message when checking coding style with checkpatch script
  rtc: s3c: Add s3c_rtc_data structure to use variant data instead of s3c_cpu_type
  rtc: s3c: Add support for RTC of Exynos3250 SoC
  ARM: dts: Fix wrong compatible string of Exynos3250 RTC dt node

 Documentation/devicetree/bindings/rtc/s3c-rtc.txt |   3 +
 arch/arm/boot/dts/exynos3250.dtsi                 |   2 +-
 drivers/rtc/rtc-s3c.c                             | 869 ++++++++++++++--------
 3 files changed, 553 insertions(+), 321 deletions(-)

-- 
1.8.0

[PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch define s3c_rtc structure including necessary variables for S3C RTC
device instead of global variables. This patch improves the readability by
removing global variables.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c | 448 ++++++++++++++++++++++++++------------------------
 1 file changed, 235 insertions(+), 213 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 4958a36..1d9e158 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -43,125 +43,135 @@ struct s3c_rtc_drv_data {
 	int cpu_type;
 };
 
-/* I have yet to find an S3C implementation with more than one
- * of these rtc blocks in */
+struct s3c_rtc {
+	struct device *dev;
+	struct rtc_device *rtc;
+
+	void __iomem *base;
+	struct clk *rtc_clk;
+	bool enabled;
 
-static struct clk *rtc_clk;
-static void __iomem *s3c_rtc_base;
-static int s3c_rtc_alarmno;
-static int s3c_rtc_tickno;
-static enum s3c_cpu_type s3c_rtc_cpu_type;
+	enum s3c_cpu_type cpu_type;
 
-static DEFINE_SPINLOCK(s3c_rtc_pie_lock);
+	int irq_alarm;
+	int irq_tick;
 
-static void s3c_rtc_alarm_clk_enable(bool enable)
+	spinlock_t pie_lock;
+	spinlock_t alarm_clk_lock;
+
+	int ticnt_save, ticnt_en_save;
+	bool wake_en;
+};
+
+static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 {
-	static DEFINE_SPINLOCK(s3c_rtc_alarm_clk_lock);
-	static bool alarm_clk_enabled;
 	unsigned long irq_flags;
 
-	spin_lock_irqsave(&s3c_rtc_alarm_clk_lock, irq_flags);
+	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
 	if (enable) {
-		if (!alarm_clk_enabled) {
-			clk_enable(rtc_clk);
-			alarm_clk_enabled = true;
+		if (!info->enabled) {
+			clk_enable(info->rtc_clk);
+			info->enabled = true;
 		}
 	} else {
-		if (alarm_clk_enabled) {
-			clk_disable(rtc_clk);
-			alarm_clk_enabled = false;
+		if (info->enabled) {
+			clk_disable(info->rtc_clk);
+			info->enabled = false;
 		}
 	}
-	spin_unlock_irqrestore(&s3c_rtc_alarm_clk_lock, irq_flags);
+	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
 }
 
 /* IRQ Handlers */
-
 static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
 {
-	struct rtc_device *rdev = id;
+	struct s3c_rtc *info = (struct s3c_rtc *)id;
 
-	clk_enable(rtc_clk);
-	rtc_update_irq(rdev, 1, RTC_AF | RTC_IRQF);
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
 
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_ALM, s3c_rtc_base + S3C2410_INTP);
+	if (info->cpu_type == TYPE_S3C64XX)
+		writeb(S3C2410_INTP_ALM, info->base + S3C2410_INTP);
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
-	s3c_rtc_alarm_clk_enable(false);
+	s3c_rtc_alarm_clk_enable(info, false);
 
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t s3c_rtc_tickirq(int irq, void *id)
 {
-	struct rtc_device *rdev = id;
+	struct s3c_rtc *info = (struct s3c_rtc *)id;
+
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_PF | RTC_IRQF);
 
-	clk_enable(rtc_clk);
-	rtc_update_irq(rdev, 1, RTC_PF | RTC_IRQF);
+	if (info->cpu_type == TYPE_S3C64XX)
+		writeb(S3C2410_INTP_TIC, info->base + S3C2410_INTP);
 
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_TIC, s3c_rtc_base + S3C2410_INTP);
+	clk_disable(info->rtc_clk);
 
-	clk_disable(rtc_clk);
 	return IRQ_HANDLED;
 }
 
 /* Update control registers */
 static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int tmp;
 
-	dev_dbg(dev, "%s: aie=%d\n", __func__, enabled);
+	if (!info->base)
+		return -EINVAL;
+
+	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
 
-	clk_enable(rtc_clk);
-	tmp = readb(s3c_rtc_base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
+	clk_enable(info->rtc_clk);
+	tmp = readb(info->base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
 
 	if (enabled)
 		tmp |= S3C2410_RTCALM_ALMEN;
 
-	writeb(tmp, s3c_rtc_base + S3C2410_RTCALM);
-	clk_disable(rtc_clk);
+	writeb(tmp, info->base + S3C2410_RTCALM);
+	clk_disable(info->rtc_clk);
 
-	s3c_rtc_alarm_clk_enable(enabled);
+	s3c_rtc_alarm_clk_enable(info, enabled);
 
 	return 0;
 }
 
-static int s3c_rtc_setfreq(struct device *dev, int freq)
+static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
 	unsigned int tmp = 0;
 	int val;
 
 	if (!is_power_of_2(freq))
 		return -EINVAL;
 
-	clk_enable(rtc_clk);
-	spin_lock_irq(&s3c_rtc_pie_lock);
+	clk_enable(info->rtc_clk);
+	spin_lock_irq(&info->pie_lock);
 
-	if (s3c_rtc_cpu_type != TYPE_S3C64XX) {
-		tmp = readb(s3c_rtc_base + S3C2410_TICNT);
+	if (info->cpu_type != TYPE_S3C64XX) {
+		tmp = readb(info->base + S3C2410_TICNT);
 		tmp &= S3C2410_TICNT_ENABLE;
 	}
 
-	val = (rtc_dev->max_user_freq / freq) - 1;
+	val = (info->rtc->max_user_freq / freq) - 1;
 
-	if (s3c_rtc_cpu_type == TYPE_S3C2416 || s3c_rtc_cpu_type == TYPE_S3C2443) {
+	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
 		tmp |= S3C2443_TICNT_PART(val);
-		writel(S3C2443_TICNT1_PART(val), s3c_rtc_base + S3C2443_TICNT1);
+		writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
 
-		if (s3c_rtc_cpu_type == TYPE_S3C2416)
-			writel(S3C2416_TICNT2_PART(val), s3c_rtc_base + S3C2416_TICNT2);
+		if (info->cpu_type == TYPE_S3C2416)
+			writel(S3C2416_TICNT2_PART(val),
+				info->base + S3C2416_TICNT2);
 	} else {
 		tmp |= val;
 	}
 
-	writel(tmp, s3c_rtc_base + S3C2410_TICNT);
-	spin_unlock_irq(&s3c_rtc_pie_lock);
-	clk_disable(rtc_clk);
+	writel(tmp, info->base + S3C2410_TICNT);
+	spin_unlock_irq(&info->pie_lock);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
@@ -170,17 +180,20 @@ static int s3c_rtc_setfreq(struct device *dev, int freq)
 
 static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int have_retried = 0;
-	void __iomem *base = s3c_rtc_base;
 
-	clk_enable(rtc_clk);
+	if (!info->base)
+		return -EINVAL;
+
+	clk_enable(info->rtc_clk);
  retry_get_time:
-	rtc_tm->tm_min  = readb(base + S3C2410_RTCMIN);
-	rtc_tm->tm_hour = readb(base + S3C2410_RTCHOUR);
-	rtc_tm->tm_mday = readb(base + S3C2410_RTCDATE);
-	rtc_tm->tm_mon  = readb(base + S3C2410_RTCMON);
-	rtc_tm->tm_year = readb(base + S3C2410_RTCYEAR);
-	rtc_tm->tm_sec  = readb(base + S3C2410_RTCSEC);
+	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
+	rtc_tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
+	rtc_tm->tm_mday = readb(info->base + S3C2410_RTCDATE);
+	rtc_tm->tm_mon  = readb(info->base + S3C2410_RTCMON);
+	rtc_tm->tm_year = readb(info->base + S3C2410_RTCYEAR);
+	rtc_tm->tm_sec  = readb(info->base + S3C2410_RTCSEC);
 
 	/* the only way to work out whether the system was mid-update
 	 * when we read it is to check the second counter, and if it
@@ -207,15 +220,19 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 
 	rtc_tm->tm_mon -= 1;
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
+
 	return rtc_valid_tm(rtc_tm);
 }
 
 static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 {
-	void __iomem *base = s3c_rtc_base;
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	int year = tm->tm_year - 100;
 
+	if (!info->base)
+		return -EINVAL;
+
 	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
 		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
@@ -227,33 +244,38 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 		return -EINVAL;
 	}
 
-	clk_enable(rtc_clk);
-	writeb(bin2bcd(tm->tm_sec),  base + S3C2410_RTCSEC);
-	writeb(bin2bcd(tm->tm_min),  base + S3C2410_RTCMIN);
-	writeb(bin2bcd(tm->tm_hour), base + S3C2410_RTCHOUR);
-	writeb(bin2bcd(tm->tm_mday), base + S3C2410_RTCDATE);
-	writeb(bin2bcd(tm->tm_mon + 1), base + S3C2410_RTCMON);
-	writeb(bin2bcd(year), base + S3C2410_RTCYEAR);
-	clk_disable(rtc_clk);
+	clk_enable(info->rtc_clk);
+
+	writeb(bin2bcd(tm->tm_sec),  info->base + S3C2410_RTCSEC);
+	writeb(bin2bcd(tm->tm_min),  info->base + S3C2410_RTCMIN);
+	writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_RTCHOUR);
+	writeb(bin2bcd(tm->tm_mday), info->base + S3C2410_RTCDATE);
+	writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_RTCMON);
+	writeb(bin2bcd(year), info->base + S3C2410_RTCYEAR);
+
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
 
 static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	struct rtc_time *alm_tm = &alrm->time;
-	void __iomem *base = s3c_rtc_base;
 	unsigned int alm_en;
 
-	clk_enable(rtc_clk);
-	alm_tm->tm_sec  = readb(base + S3C2410_ALMSEC);
-	alm_tm->tm_min  = readb(base + S3C2410_ALMMIN);
-	alm_tm->tm_hour = readb(base + S3C2410_ALMHOUR);
-	alm_tm->tm_mon  = readb(base + S3C2410_ALMMON);
-	alm_tm->tm_mday = readb(base + S3C2410_ALMDATE);
-	alm_tm->tm_year = readb(base + S3C2410_ALMYEAR);
+	if (!info->base)
+		return -EINVAL;
 
-	alm_en = readb(base + S3C2410_RTCALM);
+	clk_enable(info->rtc_clk);
+	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
+	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
+	alm_tm->tm_hour = readb(info->base + S3C2410_ALMHOUR);
+	alm_tm->tm_mon  = readb(info->base + S3C2410_ALMMON);
+	alm_tm->tm_mday = readb(info->base + S3C2410_ALMDATE);
+	alm_tm->tm_year = readb(info->base + S3C2410_ALMYEAR);
+
+	alm_en = readb(info->base + S3C2410_RTCALM);
 
 	alrm->enabled = (alm_en & S3C2410_RTCALM_ALMEN) ? 1 : 0;
 
@@ -297,65 +319,73 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	else
 		alm_tm->tm_year = -1;
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 	return 0;
 }
 
 static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	struct rtc_time *tm = &alrm->time;
-	void __iomem *base = s3c_rtc_base;
 	unsigned int alrm_en;
 
-	clk_enable(rtc_clk);
+	if (!info->base)
+		return -EINVAL;
+
+	clk_enable(info->rtc_clk);
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
 		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
 
-	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
-	writeb(0x00, base + S3C2410_RTCALM);
+	alrm_en = readb(info->base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
+	writeb(0x00, info->base + S3C2410_RTCALM);
 
 	if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
 		alrm_en |= S3C2410_RTCALM_SECEN;
-		writeb(bin2bcd(tm->tm_sec), base + S3C2410_ALMSEC);
+		writeb(bin2bcd(tm->tm_sec), info->base + S3C2410_ALMSEC);
 	}
 
 	if (tm->tm_min < 60 && tm->tm_min >= 0) {
 		alrm_en |= S3C2410_RTCALM_MINEN;
-		writeb(bin2bcd(tm->tm_min), base + S3C2410_ALMMIN);
+		writeb(bin2bcd(tm->tm_min), info->base + S3C2410_ALMMIN);
 	}
 
 	if (tm->tm_hour < 24 && tm->tm_hour >= 0) {
 		alrm_en |= S3C2410_RTCALM_HOUREN;
-		writeb(bin2bcd(tm->tm_hour), base + S3C2410_ALMHOUR);
+		writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_ALMHOUR);
 	}
 
 	dev_dbg(dev, "setting S3C2410_RTCALM to %08x\n", alrm_en);
 
-	writeb(alrm_en, base + S3C2410_RTCALM);
+	writeb(alrm_en, info->base + S3C2410_RTCALM);
 
 	s3c_rtc_setaie(dev, alrm->enabled);
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
+
 	return 0;
 }
 
 static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int ticnt;
 
-	clk_enable(rtc_clk);
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX) {
-		ticnt = readw(s3c_rtc_base + S3C2410_RTCCON);
+	if (!info->base)
+		return -EINVAL;
+
+	clk_enable(info->rtc_clk);
+	if (info->cpu_type == TYPE_S3C64XX) {
+		ticnt = readw(info->base + S3C2410_RTCCON);
 		ticnt &= S3C64XX_RTCCON_TICEN;
 	} else {
-		ticnt = readb(s3c_rtc_base + S3C2410_TICNT);
+		ticnt = readb(info->base + S3C2410_TICNT);
 		ticnt &= S3C2410_TICNT_ENABLE;
 	}
 
 	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 	return 0;
 }
 
@@ -368,63 +398,64 @@ static const struct rtc_class_ops s3c_rtcops = {
 	.alarm_irq_enable = s3c_rtc_setaie,
 };
 
-static void s3c_rtc_enable(struct platform_device *pdev, int en)
+static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 {
-	void __iomem *base = s3c_rtc_base;
 	unsigned int tmp;
 
-	if (s3c_rtc_base == NULL)
+	if (!info->base)
 		return;
 
-	clk_enable(rtc_clk);
+	clk_enable(info->rtc_clk);
 	if (!en) {
-		tmp = readw(base + S3C2410_RTCCON);
-		if (s3c_rtc_cpu_type == TYPE_S3C64XX)
+		tmp = readw(info->base + S3C2410_RTCCON);
+		if (info->cpu_type == TYPE_S3C64XX)
 			tmp &= ~S3C64XX_RTCCON_TICEN;
 		tmp &= ~S3C2410_RTCCON_RTCEN;
-		writew(tmp, base + S3C2410_RTCCON);
+		writew(tmp, info->base + S3C2410_RTCCON);
 
-		if (s3c_rtc_cpu_type != TYPE_S3C64XX) {
-			tmp = readb(base + S3C2410_TICNT);
+		if (info->cpu_type != TYPE_S3C64XX) {
+			tmp = readb(info->base + S3C2410_TICNT);
 			tmp &= ~S3C2410_TICNT_ENABLE;
-			writeb(tmp, base + S3C2410_TICNT);
+			writeb(tmp, info->base + S3C2410_TICNT);
 		}
 	} else {
 		/* re-enable the device, and check it is ok */
 
-		if ((readw(base+S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {
-			dev_info(&pdev->dev, "rtc disabled, re-enabling\n");
+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {
+			dev_info(info->dev, "rtc disabled, re-enabling\n");
 
-			tmp = readw(base + S3C2410_RTCCON);
+			tmp = readw(info->base + S3C2410_RTCCON);
 			writew(tmp | S3C2410_RTCCON_RTCEN,
-				base + S3C2410_RTCCON);
+				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {
-			dev_info(&pdev->dev, "removing RTCCON_CNTSEL\n");
+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {
+			dev_info(info->dev, "removing RTCCON_CNTSEL\n");
 
-			tmp = readw(base + S3C2410_RTCCON);
+			tmp = readw(info->base + S3C2410_RTCCON);
 			writew(tmp & ~S3C2410_RTCCON_CNTSEL,
-				base + S3C2410_RTCCON);
+				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {
-			dev_info(&pdev->dev, "removing RTCCON_CLKRST\n");
+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {
+			dev_info(info->dev, "removing RTCCON_CLKRST\n");
 
-			tmp = readw(base + S3C2410_RTCCON);
+			tmp = readw(info->base + S3C2410_RTCCON);
 			writew(tmp & ~S3C2410_RTCCON_CLKRST,
-				base + S3C2410_RTCCON);
+				info->base + S3C2410_RTCCON);
 		}
 	}
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 }
 
-static int s3c_rtc_remove(struct platform_device *dev)
+static int s3c_rtc_remove(struct platform_device *pdev)
 {
-	s3c_rtc_setaie(&dev->dev, 0);
+	struct s3c_rtc *info = platform_get_drvdata(pdev);
 
-	clk_unprepare(rtc_clk);
-	rtc_clk = NULL;
+	s3c_rtc_setaie(info->dev, 0);
+
+	clk_unprepare(info->rtc_clk);
+	info->rtc_clk = NULL;
 
 	return 0;
 }
@@ -447,73 +478,85 @@ static inline int s3c_rtc_get_driver_data(struct platform_device *pdev)
 
 static int s3c_rtc_probe(struct platform_device *pdev)
 {
-	struct rtc_device *rtc;
+	struct s3c_rtc *info = NULL;
 	struct rtc_time rtc_tm;
 	struct resource *res;
 	int ret;
 	int tmp;
 
-	dev_dbg(&pdev->dev, "%s: probe=%p\n", __func__, pdev);
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
 
 	/* find the IRQs */
-
-	s3c_rtc_tickno = platform_get_irq(pdev, 1);
-	if (s3c_rtc_tickno < 0) {
+	info->irq_tick = platform_get_irq(pdev, 1);
+	if (info->irq_tick < 0) {
 		dev_err(&pdev->dev, "no irq for rtc tick\n");
-		return s3c_rtc_tickno;
+		return info->irq_tick;
 	}
 
-	s3c_rtc_alarmno = platform_get_irq(pdev, 0);
-	if (s3c_rtc_alarmno < 0) {
+	info->dev = &pdev->dev;
+	info->cpu_type = s3c_rtc_get_driver_data(pdev);
+	spin_lock_init(&info->pie_lock);
+	spin_lock_init(&info->alarm_clk_lock);
+
+	platform_set_drvdata(pdev, info);
+
+	info->irq_alarm = platform_get_irq(pdev, 0);
+	if (info->irq_alarm < 0) {
 		dev_err(&pdev->dev, "no irq for alarm\n");
-		return s3c_rtc_alarmno;
+		return info->irq_alarm;
 	}
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: tick irq %d, alarm irq %d\n",
-		 s3c_rtc_tickno, s3c_rtc_alarmno);
+		 info->irq_tick, info->irq_alarm);
 
 	/* get the memory region */
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	s3c_rtc_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(s3c_rtc_base))
-		return PTR_ERR(s3c_rtc_base);
+	info->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(info->base))
+		return PTR_ERR(info->base);
 
-	rtc_clk = devm_clk_get(&pdev->dev, "rtc");
-	if (IS_ERR(rtc_clk)) {
+	info->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
+	if (IS_ERR(info->rtc_clk)) {
 		dev_err(&pdev->dev, "failed to find rtc clock source\n");
-		ret = PTR_ERR(rtc_clk);
-		rtc_clk = NULL;
-		return ret;
+		return PTR_ERR(info->rtc_clk);
 	}
-
-	clk_prepare_enable(rtc_clk);
+	clk_prepare_enable(info->rtc_clk);
 
 	/* check to see if everything is setup correctly */
-
-	s3c_rtc_enable(pdev, 1);
+	s3c_rtc_enable(info, 1);
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: RTCCON=%02x\n",
-		 readw(s3c_rtc_base + S3C2410_RTCCON));
+		 readw(info->base + S3C2410_RTCCON));
 
 	device_init_wakeup(&pdev->dev, 1);
 
 	/* register RTC and exit */
-
-	rtc = devm_rtc_device_register(&pdev->dev, "s3c", &s3c_rtcops,
+	info->rtc = devm_rtc_device_register(&pdev->dev, "s3c", &s3c_rtcops,
 				  THIS_MODULE);
-
-	if (IS_ERR(rtc)) {
+	if (IS_ERR(info->rtc)) {
 		dev_err(&pdev->dev, "cannot attach rtc\n");
-		ret = PTR_ERR(rtc);
+		ret = PTR_ERR(info->rtc);
 		goto err_nortc;
 	}
 
-	s3c_rtc_cpu_type = s3c_rtc_get_driver_data(pdev);
+	ret = devm_request_irq(&pdev->dev, info->irq_alarm, s3c_rtc_alarmirq,
+			  0,  "s3c2410-rtc alarm", info);
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", info->irq_alarm, ret);
+		goto err_nortc;
+	}
 
-	/* Check RTC Time */
+	ret = devm_request_irq(&pdev->dev, info->irq_tick, s3c_rtc_tickirq,
+			  0,  "s3c2410-rtc tick", info);
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", info->irq_tick, ret);
+		goto err_nortc;
+	}
 
-	s3c_rtc_gettime(NULL, &rtc_tm);
+	/* Check RTC Time */
+	s3c_rtc_gettime(&pdev->dev, &rtc_tm);
 
 	if (rtc_valid_tm(&rtc_tm)) {
 		rtc_tm.tm_year	= 100;
@@ -523,111 +566,90 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 		rtc_tm.tm_min	= 0;
 		rtc_tm.tm_sec	= 0;
 
-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);
 
 		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
 	}
 
-	if (s3c_rtc_cpu_type != TYPE_S3C2410)
-		rtc->max_user_freq = 32768;
+	if (info->cpu_type != TYPE_S3C2410)
+		info->rtc->max_user_freq = 32768;
 	else
-		rtc->max_user_freq = 128;
+		info->rtc->max_user_freq = 128;
 
-	if (s3c_rtc_cpu_type == TYPE_S3C2416 || s3c_rtc_cpu_type == TYPE_S3C2443) {
-		tmp = readw(s3c_rtc_base + S3C2410_RTCCON);
+	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
+		tmp = readw(info->base + S3C2410_RTCCON);
 		tmp |= S3C2443_RTCCON_TICSEL;
-		writew(tmp, s3c_rtc_base + S3C2410_RTCCON);
+		writew(tmp, info->base + S3C2410_RTCCON);
 	}
 
-	platform_set_drvdata(pdev, rtc);
+	s3c_rtc_setfreq(info, 1);
 
-	s3c_rtc_setfreq(&pdev->dev, 1);
-
-	ret = devm_request_irq(&pdev->dev, s3c_rtc_alarmno, s3c_rtc_alarmirq,
-			  0,  "s3c2410-rtc alarm", rtc);
-	if (ret) {
-		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
-		goto err_nortc;
-	}
-
-	ret = devm_request_irq(&pdev->dev, s3c_rtc_tickno, s3c_rtc_tickirq,
-			  0,  "s3c2410-rtc tick", rtc);
-	if (ret) {
-		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
-		goto err_nortc;
-	}
-
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 
  err_nortc:
-	s3c_rtc_enable(pdev, 0);
-	clk_disable_unprepare(rtc_clk);
+	s3c_rtc_enable(info, 0);
+	clk_disable_unprepare(info->rtc_clk);
 
 	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
-/* RTC Power management control */
-
-static int ticnt_save, ticnt_en_save;
-static bool wake_en;
 
 static int s3c_rtc_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 
-	clk_enable(rtc_clk);
+	clk_enable(info->rtc_clk);
 	/* save TICNT for anyone using periodic interrupts */
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX) {
-		ticnt_en_save = readw(s3c_rtc_base + S3C2410_RTCCON);
-		ticnt_en_save &= S3C64XX_RTCCON_TICEN;
-		ticnt_save = readl(s3c_rtc_base + S3C2410_TICNT);
+	if (info->cpu_type == TYPE_S3C64XX) {
+		info->ticnt_en_save = readw(info->base + S3C2410_RTCCON);
+		info->ticnt_en_save &= S3C64XX_RTCCON_TICEN;
+		info->ticnt_save = readl(info->base + S3C2410_TICNT);
 	} else {
-		ticnt_save = readb(s3c_rtc_base + S3C2410_TICNT);
+		info->ticnt_save = readb(info->base + S3C2410_TICNT);
 	}
-	s3c_rtc_enable(pdev, 0);
+	s3c_rtc_enable(info, 0);
 
-	if (device_may_wakeup(dev) && !wake_en) {
-		if (enable_irq_wake(s3c_rtc_alarmno) == 0)
-			wake_en = true;
+	if (device_may_wakeup(dev) && !info->wake_en) {
+		if (enable_irq_wake(info->irq_alarm) == 0)
+			info->wake_en = true;
 		else
 			dev_err(dev, "enable_irq_wake failed\n");
 	}
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
 
 static int s3c_rtc_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int tmp;
 
-	clk_enable(rtc_clk);
-	s3c_rtc_enable(pdev, 1);
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX) {
-		writel(ticnt_save, s3c_rtc_base + S3C2410_TICNT);
-		if (ticnt_en_save) {
-			tmp = readw(s3c_rtc_base + S3C2410_RTCCON);
-			writew(tmp | ticnt_en_save,
-					s3c_rtc_base + S3C2410_RTCCON);
+	clk_enable(info->rtc_clk);
+	s3c_rtc_enable(info, 1);
+	if (info->cpu_type == TYPE_S3C64XX) {
+		writel(info->ticnt_save, info->base + S3C2410_TICNT);
+		if (info->ticnt_en_save) {
+			tmp = readw(info->base + S3C2410_RTCCON);
+			writew(tmp | info->ticnt_en_save,
+					info->base + S3C2410_RTCCON);
 		}
 	} else {
-		writeb(ticnt_save, s3c_rtc_base + S3C2410_TICNT);
+		writeb(info->ticnt_save, info->base + S3C2410_TICNT);
 	}
 
-	if (device_may_wakeup(dev) && wake_en) {
-		disable_irq_wake(s3c_rtc_alarmno);
-		wake_en = false;
+	if (device_may_wakeup(dev) && info->wake_en) {
+		disable_irq_wake(info->irq_alarm);
+		info->wake_en = false;
 	}
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
 #endif
-
 static SIMPLE_DEV_PM_OPS(s3c_rtc_pm_ops, s3c_rtc_suspend, s3c_rtc_resume);
 
 #ifdef CONFIG_OF
-- 
1.8.0

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Andrew Morton]

On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:

> This patch define s3c_rtc structure including necessary variables for S3C RTC
> device instead of global variables. This patch improves the readability by
> removing global variables.

Below is the v1->v2 delta.

Why were all those tests of info->base added?  Can it really be zero? 
I don't see how.

--- a/drivers/rtc/rtc-s3c.c~rtc-s3c-define-s3c_rtc-structure-to-remove-global-variables-v2
+++ a/drivers/rtc/rtc-s3c.c
@@ -121,6 +121,9 @@ static int s3c_rtc_setaie(struct device
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int tmp;
 
+	if (!info->base)
+		return -EINVAL;
+
 	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
 
 	clk_enable(info->rtc_clk);
@@ -180,6 +183,9 @@ static int s3c_rtc_gettime(struct device
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int have_retried = 0;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
  retry_get_time:
 	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
@@ -224,6 +230,9 @@ static int s3c_rtc_settime(struct device
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	int year = tm->tm_year - 100;
 
+	if (!info->base)
+		return -EINVAL;
+
 	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
 		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
@@ -255,6 +264,9 @@ static int s3c_rtc_getalarm(struct devic
 	struct rtc_time *alm_tm = &alrm->time;
 	unsigned int alm_en;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
 	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
 	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
@@ -317,6 +329,9 @@ static int s3c_rtc_setalarm(struct devic
 	struct rtc_time *tm = &alrm->time;
 	unsigned int alrm_en;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
@@ -357,6 +372,9 @@ static int s3c_rtc_proc(struct device *d
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int ticnt;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
 	if (info->cpu_type == TYPE_S3C64XX) {
 		ticnt = readw(info->base + S3C2410_RTCCON);
@@ -548,7 +566,7 @@ static int s3c_rtc_probe(struct platform
 		rtc_tm.tm_min	= 0;
 		rtc_tm.tm_sec	= 0;
 
-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);
 
 		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
 	}
_

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Chanwoo Choi]

Dear Andrew, 

On 08/23/2014 05:42 AM, Andrew Morton wrote:
> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:
> 
>> This patch define s3c_rtc structure including necessary variables for S3C RTC
>> device instead of global variables. This patch improves the readability by
>> removing global variables.
> 
> Below is the v1->v2 delta.
> 
> Why were all those tests of info->base added?  Can it really be zero? 
> I don't see how.

If some functions (e.g., s3c_rtc_settime) accesses the rtc register
by using info->base before the initialization of info->base in s3c_rtc_probe,
I thought that null pointer error would happen.

But, I missed one point which info->base might have the garbate data instead of NULL.
I'll add the initialization code for info->base.
	info->base = NULL;

If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).

Best Regads,
Chanwoo Choi

> 
> --- a/drivers/rtc/rtc-s3c.c~rtc-s3c-define-s3c_rtc-structure-to-remove-global-variables-v2
> +++ a/drivers/rtc/rtc-s3c.c
> @@ -121,6 +121,9 @@ static int s3c_rtc_setaie(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int tmp;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
>  
>  	clk_enable(info->rtc_clk);
> @@ -180,6 +183,9 @@ static int s3c_rtc_gettime(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int have_retried = 0;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>   retry_get_time:
>  	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
> @@ -224,6 +230,9 @@ static int s3c_rtc_settime(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	int year = tm->tm_year - 100;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
>  		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
>  		 tm->tm_hour, tm->tm_min, tm->tm_sec);
> @@ -255,6 +264,9 @@ static int s3c_rtc_getalarm(struct devic
>  	struct rtc_time *alm_tm = &alrm->time;
>  	unsigned int alm_en;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
>  	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
> @@ -317,6 +329,9 @@ static int s3c_rtc_setalarm(struct devic
>  	struct rtc_time *tm = &alrm->time;
>  	unsigned int alrm_en;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
>  		 alrm->enabled,
> @@ -357,6 +372,9 @@ static int s3c_rtc_proc(struct device *d
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int ticnt;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	if (info->cpu_type == TYPE_S3C64XX) {
>  		ticnt = readw(info->base + S3C2410_RTCCON);
> @@ -548,7 +566,7 @@ static int s3c_rtc_probe(struct platform
>  		rtc_tm.tm_min	= 0;
>  		rtc_tm.tm_sec	= 0;
>  
> -		s3c_rtc_settime(NULL, &rtc_tm);
> +		s3c_rtc_settime(&pdev->dev, &rtc_tm);
>  
>  		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
>  	}
> _
> 
> 

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Andrew Morton]

On Mon, 25 Aug 2014 09:57:33 +0900 Chanwoo Choi <cw00.choi@samsung.com> wrote:

> Dear Andrew, 
> 
> On 08/23/2014 05:42 AM, Andrew Morton wrote:
> > On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:
> > 
> >> This patch define s3c_rtc structure including necessary variables for S3C RTC
> >> device instead of global variables. This patch improves the readability by
> >> removing global variables.
> > 
> > Below is the v1->v2 delta.
> > 
> > Why were all those tests of info->base added?  Can it really be zero? 
> > I don't see how.
> 
> If some functions (e.g., s3c_rtc_settime) accesses the rtc register
> by using info->base before the initialization of info->base in s3c_rtc_probe,
> I thought that null pointer error would happen.

probe() should be called before anything else.  If we're somehow
calling s3c_rtc_settime() before probe() has completed then something
very bad is happening - for example, the device may have been
registered far too early.  But I don't think that's the case here.

That being said, it does seem strange that s3c_rtc_probe() calls
devm_rtc_device_register() *before* trying to request its IRQs.  So if
IRQ requesting fails, we go and immediately unregister the device. 
Some other drivers do it this way, others do not.  Wouldn't it be
better to defer registration until we know that all the probe() setup
operations have succeeded?

> But, I missed one point which info->base might have the garbate data instead of NULL.
> I'll add the initialization code for info->base.
> 	info->base = NULL;
> 
> If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).

Well, we should have those checks in there unless we know they're
needed.  And if they *are* needed, we should have a good understanding
of why they're needed, and we should be sure that we're not just
working around some underlying problem.

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Chanwoo Choi]

Dear Andrew,

On 08/27/2014 06:31 AM, Andrew Morton wrote:
> On Mon, 25 Aug 2014 09:57:33 +0900 Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 
>> Dear Andrew, 
>>
>> On 08/23/2014 05:42 AM, Andrew Morton wrote:
>>> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:
>>>
>>>> This patch define s3c_rtc structure including necessary variables for S3C RTC
>>>> device instead of global variables. This patch improves the readability by
>>>> removing global variables.
>>>
>>> Below is the v1->v2 delta.
>>>
>>> Why were all those tests of info->base added?  Can it really be zero? 
>>> I don't see how.
>>
>> If some functions (e.g., s3c_rtc_settime) accesses the rtc register
>> by using info->base before the initialization of info->base in s3c_rtc_probe,
>> I thought that null pointer error would happen.
> 
> probe() should be called before anything else.  If we're somehow
> calling s3c_rtc_settime() before probe() has completed then something
> very bad is happening - for example, the device may have been
> registered far too early.  But I don't think that's the case here.

I means that existing rtc-s3c.c driver executed s3c_rtc_settime() in s3c_rtc_probe()
before initialization of info->base. But, It is my mistake. so, I modified it just as following:

-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);

> 
> That being said, it does seem strange that s3c_rtc_probe() calls
> devm_rtc_device_register() *before* trying to request its IRQs.  So if
> IRQ requesting fails, we go and immediately unregister the device. 
> Some other drivers do it this way, others do not.  Wouldn't it be
> better to defer registration until we know that all the probe() setup
> operations have succeeded?

You're right. I missed this point. If rtc-s3c.c driver completed the probe function,
info->base has always right address.

+	if (!info->base)
+		return -EINVAL;
+

As you said, checking state of 'info-base' is un-needed.
I'll send new patchset(v3) to fix it.

> 
>> But, I missed one point which info->base might have the garbate data instead of NULL.
>> I'll add the initialization code for info->base.
>> 	info->base = NULL;
>>
>> If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).
> 
> Well, we should have those checks in there unless we know they're
> needed.  And if they *are* needed, we should have a good understanding
> of why they're needed, and we should be sure that we're not just
> working around some underlying problem.

You are right. Thanks for your comment and advice.

Best Regards,
Chanwoo Choi

[PATCHv2 2/5] rtc: s3c: Remove warning message when checking coding style with checkpatch script

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch remove warning message when checking codeing style with checkpatch
script and reduce un-necessary i2c read operation on s3c_rtc_enable.

	WARNING: line over 80 characters
	#406: FILE: drivers/rtc/rtc-s3c.c:406:
	+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {

	WARNING: line over 80 characters
	#414: FILE: drivers/rtc/rtc-s3c.c:414:
	+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {

	WARNING: line over 80 characters
	#422: FILE: drivers/rtc/rtc-s3c.c:422:
	+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {

	WARNING: Missing a blank line after declarations
	#451: FILE: drivers/rtc/rtc-s3c.c:451:
	+	struct s3c_rtc_drv_data *data;
	+	if (pdev->dev.of_node) {

	WARNING: Missing a blank line after declarations
	#453: FILE: drivers/rtc/rtc-s3c.c:453:
	+		const struct of_device_id *match;
	+		match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);

	WARNING: DT compatible string "samsung,s3c2416-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
	#650: FILE: drivers/rtc/rtc-s3c.c:650:
	+		.compatible = "samsung,s3c2416-rtc",

	WARNING: DT compatible string "samsung,s3c2443-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
	#653: FILE: drivers/rtc/rtc-s3c.c:653:
	+		.compatible = "samsung,s3c2443-rtc",

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/rtc/s3c-rtc.txt |  2 ++
 drivers/rtc/rtc-s3c.c                             | 26 ++++++++++++-----------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
index 7ac7259..06db446 100644
--- a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
@@ -3,6 +3,8 @@
 Required properties:
 - compatible: should be one of the following.
     * "samsung,s3c2410-rtc" - for controllers compatible with s3c2410 rtc.
+    * "samsung,s3c2416-rtc" - for controllers compatible with s3c2416 rtc.
+    * "samsung,s3c2443-rtc" - for controllers compatible with s3c2443 rtc.
     * "samsung,s3c6410-rtc" - for controllers compatible with s3c6410 rtc.
 - reg: physical base address of the controller and length of memory mapped
   region.
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 1d9e158..d8f25bd 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -400,28 +400,28 @@ static const struct rtc_class_ops s3c_rtcops = {
 
 static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 {
-	unsigned int tmp;
+	unsigned int con, tmp;
 
 	if (!info->base)
 		return;
 
 	clk_enable(info->rtc_clk);
+
+	con = readw(info->base + S3C2410_RTCCON);
 	if (!en) {
-		tmp = readw(info->base + S3C2410_RTCCON);
 		if (info->cpu_type == TYPE_S3C64XX)
-			tmp &= ~S3C64XX_RTCCON_TICEN;
-		tmp &= ~S3C2410_RTCCON_RTCEN;
-		writew(tmp, info->base + S3C2410_RTCCON);
+			con &= ~S3C64XX_RTCCON_TICEN;
+		con &= ~S3C2410_RTCCON_RTCEN;
+		writew(con, info->base + S3C2410_RTCCON);
 
 		if (info->cpu_type != TYPE_S3C64XX) {
-			tmp = readb(info->base + S3C2410_TICNT);
-			tmp &= ~S3C2410_TICNT_ENABLE;
-			writeb(tmp, info->base + S3C2410_TICNT);
+			con = readb(info->base + S3C2410_TICNT);
+			con &= ~S3C2410_TICNT_ENABLE;
+			writeb(con, info->base + S3C2410_TICNT);
 		}
 	} else {
 		/* re-enable the device, and check it is ok */
-
-		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {
+		if ((con & S3C2410_RTCCON_RTCEN) == 0) {
 			dev_info(info->dev, "rtc disabled, re-enabling\n");
 
 			tmp = readw(info->base + S3C2410_RTCCON);
@@ -429,7 +429,7 @@ static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {
+		if (con & S3C2410_RTCCON_CNTSEL) {
 			dev_info(info->dev, "removing RTCCON_CNTSEL\n");
 
 			tmp = readw(info->base + S3C2410_RTCCON);
@@ -437,7 +437,7 @@ static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {
+		if (con & S3C2410_RTCCON_CLKRST) {
 			dev_info(info->dev, "removing RTCCON_CLKRST\n");
 
 			tmp = readw(info->base + S3C2410_RTCCON);
@@ -466,8 +466,10 @@ static inline int s3c_rtc_get_driver_data(struct platform_device *pdev)
 {
 #ifdef CONFIG_OF
 	struct s3c_rtc_drv_data *data;
+
 	if (pdev->dev.of_node) {
 		const struct of_device_id *match;
+
 		match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
 		data = (struct s3c_rtc_drv_data *) match->data;
 		return data->cpu_type;
-- 
1.8.0

[PATCHv2 3/5] rtc: s3c: Add s3c_rtc_data structure to use variant data instead of s3c_cpu_type

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch add s3c_rtc_data structure to variant data according to SoC type.
The s3c_rtc_data structure includes some functions to control RTC operation
and specific data dependent on SoC type.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c | 464 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 289 insertions(+), 175 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index d8f25bd..26a88f9 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -32,17 +32,6 @@
 #include <asm/irq.h>
 #include "rtc-s3c.h"
 
-enum s3c_cpu_type {
-	TYPE_S3C2410,
-	TYPE_S3C2416,
-	TYPE_S3C2443,
-	TYPE_S3C64XX,
-};
-
-struct s3c_rtc_drv_data {
-	int cpu_type;
-};
-
 struct s3c_rtc {
 	struct device *dev;
 	struct rtc_device *rtc;
@@ -51,7 +40,7 @@ struct s3c_rtc {
 	struct clk *rtc_clk;
 	bool enabled;
 
-	enum s3c_cpu_type cpu_type;
+	struct s3c_rtc_data *data;
 
 	int irq_alarm;
 	int irq_tick;
@@ -63,6 +52,19 @@ struct s3c_rtc {
 	bool wake_en;
 };
 
+struct s3c_rtc_data {
+	int max_user_freq;
+
+	void (*irq_handler) (struct s3c_rtc *info, int mask);
+	void (*set_freq) (struct s3c_rtc *info, int freq);
+	void (*enable_tick) (struct s3c_rtc *info, struct seq_file *seq);
+	void (*select_tick_clk) (struct s3c_rtc *info);
+	void (*save_tick_cnt) (struct s3c_rtc *info);
+	void (*restore_tick_cnt) (struct s3c_rtc *info);
+	void (*enable) (struct s3c_rtc *info);
+	void (*disable) (struct s3c_rtc *info);
+};
+
 static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 {
 	unsigned long irq_flags;
@@ -83,34 +85,22 @@ static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 }
 
 /* IRQ Handlers */
-static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
+static irqreturn_t s3c_rtc_tickirq(int irq, void *id)
 {
 	struct s3c_rtc *info = (struct s3c_rtc *)id;
 
-	clk_enable(info->rtc_clk);
-	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
-
-	if (info->cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_ALM, info->base + S3C2410_INTP);
-
-	clk_disable(info->rtc_clk);
-
-	s3c_rtc_alarm_clk_enable(info, false);
+	if (info->data->irq_handler)
+		info->data->irq_handler(info, S3C2410_INTP_TIC);
 
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t s3c_rtc_tickirq(int irq, void *id)
+static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
 {
 	struct s3c_rtc *info = (struct s3c_rtc *)id;
 
-	clk_enable(info->rtc_clk);
-	rtc_update_irq(info->rtc, 1, RTC_PF | RTC_IRQF);
-
-	if (info->cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_TIC, info->base + S3C2410_INTP);
-
-	clk_disable(info->rtc_clk);
+	if (info->data->irq_handler)
+		info->data->irq_handler(info, S3C2410_INTP_ALM);
 
 	return IRQ_HANDLED;
 }
@@ -140,36 +130,18 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 	return 0;
 }
 
+/* Set RTC frequency */
 static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 {
-	unsigned int tmp = 0;
-	int val;
-
 	if (!is_power_of_2(freq))
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
 	spin_lock_irq(&info->pie_lock);
 
-	if (info->cpu_type != TYPE_S3C64XX) {
-		tmp = readb(info->base + S3C2410_TICNT);
-		tmp &= S3C2410_TICNT_ENABLE;
-	}
-
-	val = (info->rtc->max_user_freq / freq) - 1;
-
-	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
-		tmp |= S3C2443_TICNT_PART(val);
-		writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
+	if (info->data->set_freq)
+		info->data->set_freq(info, freq);
 
-		if (info->cpu_type == TYPE_S3C2416)
-			writel(S3C2416_TICNT2_PART(val),
-				info->base + S3C2416_TICNT2);
-	} else {
-		tmp |= val;
-	}
-
-	writel(tmp, info->base + S3C2410_TICNT);
 	spin_unlock_irq(&info->pie_lock);
 	clk_disable(info->rtc_clk);
 
@@ -177,7 +149,6 @@ static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 }
 
 /* Time read/write */
-
 static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
@@ -370,22 +341,17 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
-	unsigned int ticnt;
 
 	if (!info->base)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
-	if (info->cpu_type == TYPE_S3C64XX) {
-		ticnt = readw(info->base + S3C2410_RTCCON);
-		ticnt &= S3C64XX_RTCCON_TICEN;
-	} else {
-		ticnt = readb(info->base + S3C2410_TICNT);
-		ticnt &= S3C2410_TICNT_ENABLE;
-	}
 
-	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
+	if (info->data->enable_tick)
+		info->data->enable_tick(info, seq);
+
 	clk_disable(info->rtc_clk);
+
 	return 0;
 }
 
@@ -398,53 +364,69 @@ static const struct rtc_class_ops s3c_rtcops = {
 	.alarm_irq_enable = s3c_rtc_setaie,
 };
 
-static void s3c_rtc_enable(struct s3c_rtc *info, int en)
+static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 {
 	unsigned int con, tmp;
 
-	if (!info->base)
-		return;
-
 	clk_enable(info->rtc_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
-	if (!en) {
-		if (info->cpu_type == TYPE_S3C64XX)
-			con &= ~S3C64XX_RTCCON_TICEN;
-		con &= ~S3C2410_RTCCON_RTCEN;
-		writew(con, info->base + S3C2410_RTCCON);
-
-		if (info->cpu_type != TYPE_S3C64XX) {
-			con = readb(info->base + S3C2410_TICNT);
-			con &= ~S3C2410_TICNT_ENABLE;
-			writeb(con, info->base + S3C2410_TICNT);
-		}
-	} else {
-		/* re-enable the device, and check it is ok */
-		if ((con & S3C2410_RTCCON_RTCEN) == 0) {
-			dev_info(info->dev, "rtc disabled, re-enabling\n");
+	/* re-enable the device, and check it is ok */
+	if ((con & S3C2410_RTCCON_RTCEN) == 0) {
+		dev_info(info->dev, "rtc disabled, re-enabling\n");
 
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp | S3C2410_RTCCON_RTCEN,
-				info->base + S3C2410_RTCCON);
-		}
+		tmp = readw(info->base + S3C2410_RTCCON);
+		writew(tmp | S3C2410_RTCCON_RTCEN,
+			info->base + S3C2410_RTCCON);
+	}
 
-		if (con & S3C2410_RTCCON_CNTSEL) {
-			dev_info(info->dev, "removing RTCCON_CNTSEL\n");
+	if (con & S3C2410_RTCCON_CNTSEL) {
+		dev_info(info->dev, "removing RTCCON_CNTSEL\n");
 
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp & ~S3C2410_RTCCON_CNTSEL,
-				info->base + S3C2410_RTCCON);
-		}
+		tmp = readw(info->base + S3C2410_RTCCON);
+		writew(tmp & ~S3C2410_RTCCON_CNTSEL,
+			info->base + S3C2410_RTCCON);
+	}
 
-		if (con & S3C2410_RTCCON_CLKRST) {
-			dev_info(info->dev, "removing RTCCON_CLKRST\n");
+	if (con & S3C2410_RTCCON_CLKRST) {
+		dev_info(info->dev, "removing RTCCON_CLKRST\n");
 
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp & ~S3C2410_RTCCON_CLKRST,
-				info->base + S3C2410_RTCCON);
-		}
+		tmp = readw(info->base + S3C2410_RTCCON);
+		writew(tmp & ~S3C2410_RTCCON_CLKRST,
+			info->base + S3C2410_RTCCON);
 	}
+
+	clk_disable(info->rtc_clk);
+}
+
+static void s3c24xx_rtc_disable(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	clk_enable(info->rtc_clk);
+
+	con = readw(info->base + S3C2410_RTCCON);
+	con &= ~S3C2410_RTCCON_RTCEN;
+	writew(con, info->base + S3C2410_RTCCON);
+
+	con = readb(info->base + S3C2410_TICNT);
+	con &= ~S3C2410_TICNT_ENABLE;
+	writeb(con, info->base + S3C2410_TICNT);
+
+	clk_disable(info->rtc_clk);
+}
+
+static void s3c6410_rtc_disable(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	clk_enable(info->rtc_clk);
+
+	con = readw(info->base + S3C2410_RTCCON);
+	con &= ~S3C64XX_RTCCON_TICEN;
+	con &= ~S3C2410_RTCCON_RTCEN;
+	writew(con, info->base + S3C2410_RTCCON);
+
 	clk_disable(info->rtc_clk);
 }
 
@@ -462,20 +444,12 @@ static int s3c_rtc_remove(struct platform_device *pdev)
 
 static const struct of_device_id s3c_rtc_dt_match[];
 
-static inline int s3c_rtc_get_driver_data(struct platform_device *pdev)
+static struct s3c_rtc_data *s3c_rtc_get_data(struct platform_device *pdev)
 {
-#ifdef CONFIG_OF
-	struct s3c_rtc_drv_data *data;
-
-	if (pdev->dev.of_node) {
-		const struct of_device_id *match;
+	const struct of_device_id *match;
 
-		match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
-		data = (struct s3c_rtc_drv_data *) match->data;
-		return data->cpu_type;
-	}
-#endif
-	return platform_get_device_id(pdev)->driver_data;
+	match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
+	return (struct s3c_rtc_data *)match->data;
 }
 
 static int s3c_rtc_probe(struct platform_device *pdev)
@@ -484,7 +458,6 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	struct rtc_time rtc_tm;
 	struct resource *res;
 	int ret;
-	int tmp;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -498,7 +471,11 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	}
 
 	info->dev = &pdev->dev;
-	info->cpu_type = s3c_rtc_get_driver_data(pdev);
+	info->data = s3c_rtc_get_data(pdev);
+	if (!info->data) {
+		dev_err(&pdev->dev, "failed getting s3c_rtc_data\n");
+		return -EINVAL;
+	}
 	spin_lock_init(&info->pie_lock);
 	spin_lock_init(&info->alarm_clk_lock);
 
@@ -527,7 +504,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	clk_prepare_enable(info->rtc_clk);
 
 	/* check to see if everything is setup correctly */
-	s3c_rtc_enable(info, 1);
+	if (info->data->enable)
+		info->data->enable(info);
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: RTCCON=%02x\n",
 		 readw(info->base + S3C2410_RTCCON));
@@ -573,16 +551,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
 	}
 
-	if (info->cpu_type != TYPE_S3C2410)
-		info->rtc->max_user_freq = 32768;
-	else
-		info->rtc->max_user_freq = 128;
-
-	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
-		tmp = readw(info->base + S3C2410_RTCCON);
-		tmp |= S3C2443_RTCCON_TICSEL;
-		writew(tmp, info->base + S3C2410_RTCCON);
-	}
+	if (info->data->select_tick_clk)
+		info->data->select_tick_clk(info);
 
 	s3c_rtc_setfreq(info, 1);
 
@@ -591,7 +561,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	return 0;
 
  err_nortc:
-	s3c_rtc_enable(info, 0);
+	if (info->data->disable)
+		info->data->disable(info);
 	clk_disable_unprepare(info->rtc_clk);
 
 	return ret;
@@ -604,15 +575,13 @@ static int s3c_rtc_suspend(struct device *dev)
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 
 	clk_enable(info->rtc_clk);
+
 	/* save TICNT for anyone using periodic interrupts */
-	if (info->cpu_type == TYPE_S3C64XX) {
-		info->ticnt_en_save = readw(info->base + S3C2410_RTCCON);
-		info->ticnt_en_save &= S3C64XX_RTCCON_TICEN;
-		info->ticnt_save = readl(info->base + S3C2410_TICNT);
-	} else {
-		info->ticnt_save = readb(info->base + S3C2410_TICNT);
-	}
-	s3c_rtc_enable(info, 0);
+	if (info->data->save_tick_cnt)
+		info->data->save_tick_cnt(info);
+
+	if (info->data->disable)
+		info->data->disable(info);
 
 	if (device_may_wakeup(dev) && !info->wake_en) {
 		if (enable_irq_wake(info->irq_alarm) == 0)
@@ -620,6 +589,7 @@ static int s3c_rtc_suspend(struct device *dev)
 		else
 			dev_err(dev, "enable_irq_wake failed\n");
 	}
+
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -628,25 +598,20 @@ static int s3c_rtc_suspend(struct device *dev)
 static int s3c_rtc_resume(struct device *dev)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
-	unsigned int tmp;
 
 	clk_enable(info->rtc_clk);
-	s3c_rtc_enable(info, 1);
-	if (info->cpu_type == TYPE_S3C64XX) {
-		writel(info->ticnt_save, info->base + S3C2410_TICNT);
-		if (info->ticnt_en_save) {
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp | info->ticnt_en_save,
-					info->base + S3C2410_RTCCON);
-		}
-	} else {
-		writeb(info->ticnt_save, info->base + S3C2410_TICNT);
-	}
+
+	if (info->data->enable)
+		info->data->enable(info);
+
+	if (info->data->restore_tick_cnt)
+		info->data->restore_tick_cnt(info);
 
 	if (device_may_wakeup(dev) && info->wake_en) {
 		disable_irq_wake(info->irq_alarm);
 		info->wake_en = false;
 	}
+
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -654,56 +619,206 @@ static int s3c_rtc_resume(struct device *dev)
 #endif
 static SIMPLE_DEV_PM_OPS(s3c_rtc_pm_ops, s3c_rtc_suspend, s3c_rtc_resume);
 
-#ifdef CONFIG_OF
-static struct s3c_rtc_drv_data s3c_rtc_drv_data_array[] = {
-	[TYPE_S3C2410] = { TYPE_S3C2410 },
-	[TYPE_S3C2416] = { TYPE_S3C2416 },
-	[TYPE_S3C2443] = { TYPE_S3C2443 },
-	[TYPE_S3C64XX] = { TYPE_S3C64XX },
+static void s3c24xx_rtc_irq(struct s3c_rtc *info, int mask)
+{
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
+	clk_disable(info->rtc_clk);
+
+	s3c_rtc_alarm_clk_enable(info, false);
+}
+
+static void s3c6410_rtc_irq(struct s3c_rtc *info, int mask)
+{
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
+	writeb(mask, info->base + S3C2410_INTP);
+	clk_disable(info->rtc_clk);
+
+	s3c_rtc_alarm_clk_enable(info, false);
+}
+
+static void s3c2410_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	unsigned int tmp = 0;
+	int val;
+
+	tmp = readb(info->base + S3C2410_TICNT);
+	tmp &= S3C2410_TICNT_ENABLE;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+	tmp |= val;
+
+	writel(tmp, info->base + S3C2410_TICNT);
+}
+
+static void s3c2416_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	unsigned int tmp = 0;
+	int val;
+
+	tmp = readb(info->base + S3C2410_TICNT);
+	tmp &= S3C2410_TICNT_ENABLE;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+
+	tmp |= S3C2443_TICNT_PART(val);
+	writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
+
+	writel(S3C2416_TICNT2_PART(val), info->base + S3C2416_TICNT2);
+
+	writel(tmp, info->base + S3C2410_TICNT);
+}
+
+static void s3c2443_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	unsigned int tmp = 0;
+	int val;
+
+	tmp = readb(info->base + S3C2410_TICNT);
+	tmp &= S3C2410_TICNT_ENABLE;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+
+	tmp |= S3C2443_TICNT_PART(val);
+	writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
+
+	writel(tmp, info->base + S3C2410_TICNT);
+}
+
+static void s3c6410_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	int val;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+	writel(val, info->base + S3C2410_TICNT);
+}
+
+static void s3c24xx_rtc_enable_tick(struct s3c_rtc *info, struct seq_file *seq)
+{
+	unsigned int ticnt;
+
+	ticnt = readb(info->base + S3C2410_TICNT);
+	ticnt &= S3C2410_TICNT_ENABLE;
+
+	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
+}
+
+static void s3c2416_rtc_select_tick_clk(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	con = readw(info->base + S3C2410_RTCCON);
+	con |= S3C2443_RTCCON_TICSEL;
+	writew(con, info->base + S3C2410_RTCCON);
+}
+
+static void s3c6410_rtc_enable_tick(struct s3c_rtc *info, struct seq_file *seq)
+{
+	unsigned int ticnt;
+
+	ticnt = readw(info->base + S3C2410_RTCCON);
+	ticnt &= S3C64XX_RTCCON_TICEN;
+
+	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
+}
+
+static void s3c24xx_rtc_save_tick_cnt(struct s3c_rtc *info)
+{
+	info->ticnt_save = readb(info->base + S3C2410_TICNT);
+}
+
+static void s3c24xx_rtc_restore_tick_cnt(struct s3c_rtc *info)
+{
+	writeb(info->ticnt_save, info->base + S3C2410_TICNT);
+}
+
+static void s3c6410_rtc_save_tick_cnt(struct s3c_rtc *info)
+{
+	info->ticnt_en_save = readw(info->base + S3C2410_RTCCON);
+	info->ticnt_en_save &= S3C64XX_RTCCON_TICEN;
+	info->ticnt_save = readl(info->base + S3C2410_TICNT);
+}
+
+static void s3c6410_rtc_restore_tick_cnt(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	writel(info->ticnt_save, info->base + S3C2410_TICNT);
+	if (info->ticnt_en_save) {
+		con = readw(info->base + S3C2410_RTCCON);
+		writew(con | info->ticnt_en_save,
+				info->base + S3C2410_RTCCON);
+	}
+}
+
+static struct s3c_rtc_data const s3c2410_rtc_data = {
+	.max_user_freq		= 128,
+	.irq_handler		= s3c24xx_rtc_irq,
+	.set_freq		= s3c2410_rtc_setfreq,
+	.enable_tick		= s3c24xx_rtc_enable_tick,
+	.save_tick_cnt		= s3c24xx_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c24xx_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c24xx_rtc_disable,
+};
+
+static struct s3c_rtc_data const s3c2416_rtc_data = {
+	.max_user_freq		= 32768,
+	.irq_handler		= s3c24xx_rtc_irq,
+	.set_freq		= s3c2416_rtc_setfreq,
+	.enable_tick		= s3c24xx_rtc_enable_tick,
+	.select_tick_clk	= s3c2416_rtc_select_tick_clk,
+	.save_tick_cnt		= s3c24xx_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c24xx_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c24xx_rtc_disable,
+};
+
+static struct s3c_rtc_data const s3c2443_rtc_data = {
+	.max_user_freq		= 32768,
+	.irq_handler		= s3c24xx_rtc_irq,
+	.set_freq		= s3c2443_rtc_setfreq,
+	.enable_tick		= s3c24xx_rtc_enable_tick,
+	.select_tick_clk	= s3c2416_rtc_select_tick_clk,
+	.save_tick_cnt		= s3c24xx_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c24xx_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c24xx_rtc_disable,
+};
+
+static struct s3c_rtc_data const s3c6410_rtc_data = {
+	.max_user_freq		= 32768,
+	.irq_handler		= s3c6410_rtc_irq,
+	.set_freq		= s3c6410_rtc_setfreq,
+	.enable_tick		= s3c6410_rtc_enable_tick,
+	.save_tick_cnt		= s3c6410_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c6410_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c6410_rtc_disable,
 };
 
 static const struct of_device_id s3c_rtc_dt_match[] = {
 	{
 		.compatible = "samsung,s3c2410-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C2410],
+		.data = (void *)&s3c2410_rtc_data,
 	}, {
 		.compatible = "samsung,s3c2416-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C2416],
+		.data = (void *)&s3c2416_rtc_data,
 	}, {
 		.compatible = "samsung,s3c2443-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C2443],
+		.data = (void *)&s3c2443_rtc_data,
 	}, {
 		.compatible = "samsung,s3c6410-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C64XX],
+		.data = (void *)&s3c6410_rtc_data,
 	},
-	{},
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, s3c_rtc_dt_match);
-#endif
-
-static struct platform_device_id s3c_rtc_driver_ids[] = {
-	{
-		.name		= "s3c2410-rtc",
-		.driver_data	= TYPE_S3C2410,
-	}, {
-		.name		= "s3c2416-rtc",
-		.driver_data	= TYPE_S3C2416,
-	}, {
-		.name		= "s3c2443-rtc",
-		.driver_data	= TYPE_S3C2443,
-	}, {
-		.name		= "s3c64xx-rtc",
-		.driver_data	= TYPE_S3C64XX,
-	},
-	{ }
-};
-
-MODULE_DEVICE_TABLE(platform, s3c_rtc_driver_ids);
 
 static struct platform_driver s3c_rtc_driver = {
 	.probe		= s3c_rtc_probe,
 	.remove		= s3c_rtc_remove,
-	.id_table	= s3c_rtc_driver_ids,
 	.driver		= {
 		.name	= "s3c-rtc",
 		.owner	= THIS_MODULE,
@@ -711,7 +826,6 @@ static struct platform_driver s3c_rtc_driver = {
 		.of_match_table	= of_match_ptr(s3c_rtc_dt_match),
 	},
 };
-
 module_platform_driver(s3c_rtc_driver);
 
 MODULE_DESCRIPTION("Samsung S3C RTC Driver");
-- 
1.8.0

[PATCHv2 4/5] rtc: s3c: Add support for RTC of Exynos3250 SoC

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch add support for RTC of Exynos3250 SoC. The Exynos3250 needs source
clock(32.768KHz) for RTC block. If source clock of RTC is registerd on clock
list of common clk framework, Exynos RTC drvier have to control this clock.

Clock list for s3c-rtc device:
- rtc : CLK_RTC of CLK_GATE_IP_PERIR is gate clock for RTC.
- rtc_src : XrtcXTI is 32.768.kHz source clock for RTC.
 (XRTCXTI: Specifies a clock from 32.768 kHz crystal pad with XRTCXTI and
 XRTCXTO pins. RTC uses this clock as the source of a real-time clock.)

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/rtc/s3c-rtc.txt |  1 +
 drivers/rtc/rtc-s3c.c                             | 93 ++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
index 06db446..ab757b84 100644
--- a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
@@ -6,6 +6,7 @@ Required properties:
     * "samsung,s3c2416-rtc" - for controllers compatible with s3c2416 rtc.
     * "samsung,s3c2443-rtc" - for controllers compatible with s3c2443 rtc.
     * "samsung,s3c6410-rtc" - for controllers compatible with s3c6410 rtc.
+    * "samsung,exynos3250-rtc" - for controllers compatible with exynos3250 rtc.
 - reg: physical base address of the controller and length of memory mapped
   region.
 - interrupts: Two interrupt numbers to the cpu should be specified. First
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 26a88f9..90dcf51 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -38,6 +38,7 @@ struct s3c_rtc {
 
 	void __iomem *base;
 	struct clk *rtc_clk;
+	struct clk *rtc_src_clk;
 	bool enabled;
 
 	struct s3c_rtc_data *data;
@@ -54,6 +55,7 @@ struct s3c_rtc {
 
 struct s3c_rtc_data {
 	int max_user_freq;
+	bool needs_src_clk;
 
 	void (*irq_handler) (struct s3c_rtc *info, int mask);
 	void (*set_freq) (struct s3c_rtc *info, int freq);
@@ -73,10 +75,14 @@ static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 	if (enable) {
 		if (!info->enabled) {
 			clk_enable(info->rtc_clk);
+			if (info->data->needs_src_clk)
+				clk_enable(info->rtc_src_clk);
 			info->enabled = true;
 		}
 	} else {
 		if (info->enabled) {
+			if (info->data->needs_src_clk)
+				clk_disable(info->rtc_src_clk);
 			clk_disable(info->rtc_clk);
 			info->enabled = false;
 		}
@@ -117,12 +123,16 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	tmp = readb(info->base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
 
 	if (enabled)
 		tmp |= S3C2410_RTCALM_ALMEN;
 
 	writeb(tmp, info->base + S3C2410_RTCALM);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	s3c_rtc_alarm_clk_enable(info, enabled);
@@ -137,12 +147,16 @@ static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	spin_lock_irq(&info->pie_lock);
 
 	if (info->data->set_freq)
 		info->data->set_freq(info, freq);
 
 	spin_unlock_irq(&info->pie_lock);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -158,6 +172,9 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
+
  retry_get_time:
 	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
 	rtc_tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
@@ -191,6 +208,8 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 
 	rtc_tm->tm_mon -= 1;
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return rtc_valid_tm(rtc_tm);
@@ -216,6 +235,8 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	}
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	writeb(bin2bcd(tm->tm_sec),  info->base + S3C2410_RTCSEC);
 	writeb(bin2bcd(tm->tm_min),  info->base + S3C2410_RTCMIN);
@@ -224,6 +245,8 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_RTCMON);
 	writeb(bin2bcd(year), info->base + S3C2410_RTCYEAR);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -239,6 +262,9 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
+
 	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
 	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
 	alm_tm->tm_hour = readb(info->base + S3C2410_ALMHOUR);
@@ -290,7 +316,10 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	else
 		alm_tm->tm_year = -1;
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
+
 	return 0;
 }
 
@@ -304,6 +333,9 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
+
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
 		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
@@ -333,6 +365,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	s3c_rtc_setaie(dev, alrm->enabled);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -346,10 +380,14 @@ static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	if (info->data->enable_tick)
 		info->data->enable_tick(info, seq);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -369,6 +407,8 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 	unsigned int con, tmp;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
 	/* re-enable the device, and check it is ok */
@@ -396,6 +436,8 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 			info->base + S3C2410_RTCCON);
 	}
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 }
 
@@ -404,6 +446,8 @@ static void s3c24xx_rtc_disable(struct s3c_rtc *info)
 	unsigned int con;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
 	con &= ~S3C2410_RTCCON_RTCEN;
@@ -413,6 +457,8 @@ static void s3c24xx_rtc_disable(struct s3c_rtc *info)
 	con &= ~S3C2410_TICNT_ENABLE;
 	writeb(con, info->base + S3C2410_TICNT);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 }
 
@@ -421,12 +467,16 @@ static void s3c6410_rtc_disable(struct s3c_rtc *info)
 	unsigned int con;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
 	con &= ~S3C64XX_RTCCON_TICEN;
 	con &= ~S3C2410_RTCCON_RTCEN;
 	writew(con, info->base + S3C2410_RTCCON);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 }
 
@@ -498,11 +548,19 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	info->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
 	if (IS_ERR(info->rtc_clk)) {
-		dev_err(&pdev->dev, "failed to find rtc clock source\n");
+		dev_err(&pdev->dev, "failed to find rtc clock\n");
 		return PTR_ERR(info->rtc_clk);
 	}
 	clk_prepare_enable(info->rtc_clk);
 
+	info->rtc_src_clk = devm_clk_get(&pdev->dev, "rtc_src");
+	if (IS_ERR(info->rtc_src_clk)) {
+		dev_err(&pdev->dev, "failed to find rtc source clock\n");
+		return PTR_ERR(info->rtc_src_clk);
+	}
+	clk_prepare_enable(info->rtc_src_clk);
+
+
 	/* check to see if everything is setup correctly */
 	if (info->data->enable)
 		info->data->enable(info);
@@ -556,6 +614,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	s3c_rtc_setfreq(info, 1);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -575,6 +635,8 @@ static int s3c_rtc_suspend(struct device *dev)
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	/* save TICNT for anyone using periodic interrupts */
 	if (info->data->save_tick_cnt)
@@ -590,6 +652,8 @@ static int s3c_rtc_suspend(struct device *dev)
 			dev_err(dev, "enable_irq_wake failed\n");
 	}
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -600,6 +664,8 @@ static int s3c_rtc_resume(struct device *dev)
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	if (info->data->enable)
 		info->data->enable(info);
@@ -612,6 +678,8 @@ static int s3c_rtc_resume(struct device *dev)
 		info->wake_en = false;
 	}
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -622,7 +690,11 @@ static SIMPLE_DEV_PM_OPS(s3c_rtc_pm_ops, s3c_rtc_suspend, s3c_rtc_resume);
 static void s3c24xx_rtc_irq(struct s3c_rtc *info, int mask)
 {
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	s3c_rtc_alarm_clk_enable(info, false);
@@ -631,8 +703,12 @@ static void s3c24xx_rtc_irq(struct s3c_rtc *info, int mask)
 static void s3c6410_rtc_irq(struct s3c_rtc *info, int mask)
 {
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
 	writeb(mask, info->base + S3C2410_INTP);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	s3c_rtc_alarm_clk_enable(info, false);
@@ -798,6 +874,18 @@ static struct s3c_rtc_data const s3c6410_rtc_data = {
 	.disable		= s3c6410_rtc_disable,
 };
 
+static struct s3c_rtc_data const exynos3250_rtc_data = {
+	.max_user_freq		= 32768,
+	.needs_src_clk		= true,
+	.irq_handler		= s3c6410_rtc_irq,
+	.set_freq		= s3c6410_rtc_setfreq,
+	.enable_tick		= s3c6410_rtc_enable_tick,
+	.save_tick_cnt		= s3c6410_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c6410_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c6410_rtc_disable,
+};
+
 static const struct of_device_id s3c_rtc_dt_match[] = {
 	{
 		.compatible = "samsung,s3c2410-rtc",
@@ -811,6 +899,9 @@ static const struct of_device_id s3c_rtc_dt_match[] = {
 	}, {
 		.compatible = "samsung,s3c6410-rtc",
 		.data = (void *)&s3c6410_rtc_data,
+	}, {
+		.compatible = "samsung,exynos3250-rtc",
+		.data = (void *)&exynos3250_rtc_data,
 	},
 	{ /* sentinel */ },
 };
-- 
1.8.0

[PATCHv2 5/5] ARM: dts: Fix wrong compatible string of Exynos3250 RTC dt node

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch fix wrong compatible string of Exynos3250 RTC (Real-Time Clock) dt
node. The RTC of Exynos3250 must need additional source clock (XrtcXTI).

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos3250.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
index cd6a69a..1be5ef8 100644
--- a/arch/arm/boot/dts/exynos3250.dtsi
+++ b/arch/arm/boot/dts/exynos3250.dtsi
@@ -192,7 +192,7 @@
 		};
 
 		rtc: rtc@10070000 {
-			compatible = "samsung,s3c6410-rtc";
+			compatible = "samsung,exynos3250-rtc";
 			reg = <0x10070000 0x100>;
 			interrupts = <0 73 0>, <0 74 0>;
 			status = "disabled";
-- 
1.8.0

[PATCH 3/6] net: thunderx: Increase transmit queue length

[Sunil Goutham]

From: Sunil Goutham <sgoutham@cavium.com>

Under high transmit rates and with TSO enabled observing fluctuations
in TX performance. Seen especially with iperf3 application.

Since TSO is taken care at driver level, with 64KB of TSO packets
and when window size is also high the rate at which CPU fills in
transmit descriptors is much higher than what HW is able to process.
Each 64KB TSO packet occupies gets segmented to ~43 1500 byte MTU packets
and occupies ~130 descriptors. Hence increasing transmit queue size.

Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nicvf_queues.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
index fb4957d..b1e93a9 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
@@ -62,7 +62,7 @@
 #define SND_QUEUE_CNT		8
 #define CMP_QUEUE_CNT		8 /* Max of RCV and SND qcount */
 
-#define SND_QSIZE		SND_QUEUE_SIZE2
+#define SND_QSIZE		SND_QUEUE_SIZE3
 #define SND_QUEUE_LEN		(1ULL << (SND_QSIZE + 10))
 #define MAX_SND_QUEUE_LEN	(1ULL << (SND_QUEUE_SIZE6 + 10))
 #define SND_QUEUE_THRESH	2ULL
@@ -73,7 +73,7 @@
 /* Keep CQ and SQ sizes same, if timestamping
  * is enabled this equation will change.
  */
-#define CMP_QSIZE		CMP_QUEUE_SIZE2
+#define CMP_QSIZE		CMP_QUEUE_SIZE3
 #define CMP_QUEUE_LEN		(1ULL << (CMP_QSIZE + 10))
 #define CMP_QUEUE_CQE_THRESH	0
 #define CMP_QUEUE_TIMER_THRESH	220 /* 10usec */
-- 
1.7.1

RE: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Pavel Fedin]

 Hello!

 This one breaks networking on my machine:
--- cut ---
swiotlb: coherent allocation failed for device 0002:01:08.4 size=4198400
CPU: 2 PID: 3655 Comm: NetworkManager Tainted: G        W  O    4.2.6+ #201
Hardware name: Cavium ThunderX CN88XX board (DT)
Call trace:
[<ffffffc00008a3d4>] dump_backtrace+0x0/0x124
[<ffffffc00008a50c>] show_stack+0x14/0x1c
[<ffffffc0006df258>] dump_stack+0x88/0xc8
[<ffffffc000416994>] swiotlb_alloc_coherent+0x150/0x168
[<ffffffc00009555c>] __dma_alloc+0x58/0x1e8
[<ffffffc000507cd8>] nicvf_alloc_q_desc_mem.isra.14+0xc0/0xdc
[<ffffffc000508c8c>] nicvf_config_data_transfer+0x448/0x728
[<ffffffc000506c64>] nicvf_open+0x184/0x994
[<ffffffc0005ea0b4>] __dev_open+0xb4/0x120
[<ffffffc0005ea388>] __dev_change_flags+0x8c/0x150
[<ffffffc0005ea46c>] dev_change_flags+0x20/0x5c
[<ffffffc0005fb210>] do_setlink+0x27c/0x8e0
[<ffffffc0005fbe28>] rtnl_newlink+0x4b4/0x6c4
[<ffffffc0005fa6f0>] rtnetlink_rcv_msg+0x90/0x22c
[<ffffffc000611b74>] netlink_rcv_skb+0xd8/0x100
[<ffffffc0005fa650>] rtnetlink_rcv+0x30/0x40
[<ffffffc000611498>] netlink_unicast+0x158/0x1f8
[<ffffffc000611910>] netlink_sendmsg+0x324/0x380
[<ffffffc0005c6690>] sock_sendmsg+0x18/0x58
[<ffffffc0005c6db8>] ___sys_sendmsg+0x254/0x264
[<ffffffc0005c7bb8>] __sys_sendmsg+0x40/0x84
[<ffffffc0005c7c0c>] SyS_sendmsg+0x10/0x20
thunder-nicvf 0002:01:08.4 enP2p1s8f4: Failed to alloc/config VF's QSet resources
--- cut ---

 And none of interfaces work after this. Reverting this patch helps.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Tuesday, December 01, 2015 12:14 PM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
> Subject: [PATCH 3/6] net: thunderx: Increase transmit queue length
> 
> From: Sunil Goutham <sgoutham@cavium.com>
> 
> Under high transmit rates and with TSO enabled observing fluctuations
> in TX performance. Seen especially with iperf3 application.
> 
> Since TSO is taken care at driver level, with 64KB of TSO packets
> and when window size is also high the rate at which CPU fills in
> transmit descriptors is much higher than what HW is able to process.
> Each 64KB TSO packet occupies gets segmented to ~43 1500 byte MTU packets
> and occupies ~130 descriptors. Hence increasing transmit queue size.
> 
> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> ---
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> index fb4957d..b1e93a9 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> @@ -62,7 +62,7 @@
>  #define SND_QUEUE_CNT		8
>  #define CMP_QUEUE_CNT		8 /* Max of RCV and SND qcount */
> 
> -#define SND_QSIZE		SND_QUEUE_SIZE2
> +#define SND_QSIZE		SND_QUEUE_SIZE3
>  #define SND_QUEUE_LEN		(1ULL << (SND_QSIZE + 10))
>  #define MAX_SND_QUEUE_LEN	(1ULL << (SND_QUEUE_SIZE6 + 10))
>  #define SND_QUEUE_THRESH	2ULL
> @@ -73,7 +73,7 @@
>  /* Keep CQ and SQ sizes same, if timestamping
>   * is enabled this equation will change.
>   */
> -#define CMP_QSIZE		CMP_QUEUE_SIZE2
> +#define CMP_QSIZE		CMP_QUEUE_SIZE3
>  #define CMP_QUEUE_LEN		(1ULL << (CMP_QSIZE + 10))
>  #define CMP_QUEUE_CQE_THRESH	0
>  #define CMP_QUEUE_TIMER_THRESH	220 /* 10usec */
> --
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Eric Dumazet]

On Tue, 2015-12-01 at 17:40 +0300, Pavel Fedin wrote:
>  Hello!
> 
>  This one breaks networking on my machine:
> --- cut ---
> swiotlb: coherent allocation failed for device 0002:01:08.4 size=4198400
> CPU: 2 PID: 3655 Comm: NetworkManager Tainted: G        W  O    4.2.6+ #201
> Hardware name: Cavium ThunderX CN88XX

Are you sure 4.2.6 kernel is suitable for backporting this patch aimed
for linux-4.5 ?

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Sunil Kovvuri]

Hi Pavel Fedin,

Increasing descriptor ring size will lead to more memory allocation.
And what you are seeing is a memory alloc failure and doesn't seem to
be due to this driver change.
I mean it looks like the behavior will be same for other drivers as well.

Probably you might have to set "coherent_pool" size in bootargs to a
higher value.
Can you please check.

Thanks,
Sunil.

On Tue, Dec 1, 2015 at 9:03 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-12-01 at 17:40 +0300, Pavel Fedin wrote:
>>  Hello!
>>
>>  This one breaks networking on my machine:
>> --- cut ---
>> swiotlb: coherent allocation failed for device 0002:01:08.4 size=4198400
>> CPU: 2 PID: 3655 Comm: NetworkManager Tainted: G        W  O    4.2.6+ #201
>> Hardware name: Cavium ThunderX CN88XX
>
> Are you sure 4.2.6 kernel is suitable for backporting this patch aimed
> for linux-4.5 ?
>
>
>

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[David Miller]

From: Sunil Kovvuri <sunil.kovvuri@gmail.com>
Date: Tue, 1 Dec 2015 22:00:49 +0530

> Increasing descriptor ring size will lead to more memory allocation.
> And what you are seeing is a memory alloc failure and doesn't seem
> to be due to this driver change.  I mean it looks like the behavior
> will be same for other drivers as well.

The driver should successfully recover from out of memory situations
and not stop RX/TX completely.

There might be some other problem with your driver causing this.

Don't put this off as not "related" to your patch, it is as this
introduces the behavior for this user, and you should fix it before
expecting me to apply this patch series.

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Sunil Kovvuri]

>The driver should successfully recover from out of memory situations
> and not stop RX/TX completely.
This memory allocation is while interface bringup/initialization and not during
packet I/O.

>Don't put this off as not "related" to your patch, it is as this
>introduces the behavior for this user, and you should fix it before
>expecting me to apply this patch series.
I would disagree on this, as this patch hasn't introduced any failure here,
if this user has connected any device which asks for a bit large amount
of coherent memory then i am sure he will see the same issue.
And above i have suggested what could be done to not see this issue.

But anyway for the time being I will resubmit the patch series without this
patch, hope that would be okay.

On Wed, Dec 2, 2015 at 1:00 AM, David Miller <davem@davemloft.net> wrote:
> From: Sunil Kovvuri <sunil.kovvuri@gmail.com>
> Date: Tue, 1 Dec 2015 22:00:49 +0530
>
>> Increasing descriptor ring size will lead to more memory allocation.
>> And what you are seeing is a memory alloc failure and doesn't seem
>> to be due to this driver change.  I mean it looks like the behavior
>> will be same for other drivers as well.
>
> The driver should successfully recover from out of memory situations
> and not stop RX/TX completely.
>
> There might be some other problem with your driver causing this.
>
> Don't put this off as not "related" to your patch, it is as this
> introduces the behavior for this user, and you should fix it before
> expecting me to apply this patch series.

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Eric Dumazet]

On Wed, 2015-12-02 at 11:18 +0530, Sunil Kovvuri wrote:
> >The driver should successfully recover from out of memory situations
> > and not stop RX/TX completely.
> This memory allocation is while interface bringup/initialization and not during
> packet I/O.
> 
> >Don't put this off as not "related" to your patch, it is as this
> >introduces the behavior for this user, and you should fix it before
> >expecting me to apply this patch series.
> I would disagree on this, as this patch hasn't introduced any failure here,
> if this user has connected any device which asks for a bit large amount
> of coherent memory then i am sure he will see the same issue.
> And above i have suggested what could be done to not see this issue.


This is unacceptable.

Maybe you did not complete tests. changelog has no 'Tested:' section.

You can not claim this patch was good, especially considering no precise
numbers were given.

If the performance increase is 4 %, then surely using twice more memory
is not worth it.

RX/TX ring buffer sizes should be :

- Default to reasonable sizes (ie not gigantic memory usage for typical
use). For multiqueue devices, one also has to take into account the
number of queues: 

  If a 10Gbit NIC has 128 queues, then probably having 8192 slots
  per queue is too much, if this maps to 512 MB of memory !

- ethtool -G support to let the admin change the conservative settings
for the exceptional workloads.

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Sunil Kovvuri]

>
> If the performance increase is 4 %, then surely using twice more memory
> is not worth it.
I haven't mentioned anywhere that i am seeing 4% increase in performance.
Anyways as said already i will recheck and resubmit.

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Eric Dumazet]

On Wed, 2015-12-02 at 22:20 +0530, Sunil Kovvuri wrote:
> >
> > If the performance increase is 4 %, then surely using twice more memory
> > is not worth it.
> I haven't mentioned anywhere that i am seeing 4% increase in performance.

Yes, this is the complain I made :

No numbers, just a patch increasing ring size by a 100% factor !

I really hope you'll have very convincing numbers, especially if your
driver does not implement BQL.

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[David Miller]

From: Sunil Kovvuri <sunil.kovvuri@gmail.com>
Date: Wed, 2 Dec 2015 11:18:43 +0530

>>The driver should successfully recover from out of memory situations
>> and not stop RX/TX completely.
> This memory allocation is while interface bringup/initialization and not during
> packet I/O.
> 
>>Don't put this off as not "related" to your patch, it is as this
>>introduces the behavior for this user, and you should fix it before
>>expecting me to apply this patch series.
> I would disagree on this, as this patch hasn't introduced any failure here,
> if this user has connected any device which asks for a bit large amount
> of coherent memory then i am sure he will see the same issue.

It's not the memory allocation that's the problem.

It's the the device completely dies and does not recover even when
memory does become available later.

That is a hard regression which this change introduces.

RE: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Pavel Fedin]

 Hello!

> Probably you might have to set "coherent_pool" size in bootargs to a
> higher value.
> Can you please check.

 I have tried to do this. I was able to enlarge the pool up to 4MB, and still got allocation failures. At 8MB pool preallocation stops working:
--- cut ---
Call trace:
[<ffffffc00012ddb8>] __alloc_pages_nodemask+0x4f4/0x7d4
[<ffffffc0007be370>] atomic_pool_init+0x60/0x1a4
[<ffffffc0007be4d4>] arm64_dma_init+0x20/0x28
[<ffffffc000082848>] do_one_initcall+0x8c/0x1a4
[<ffffffc0007baac0>] kernel_init_freeable+0x154/0x1f4
[<ffffffc0005c2b14>] kernel_init+0x10/0xd8
DMA: failed to allocate 8192 KiB pool for atomic coherent allocation
--- cut ---
 and i get even worse faults in the driver.

 I know that it is possible to allocate larger pools by setting CONFIG_FORCE_MAX_ZONEORDER, but:
a) This is done on per-platform basis. For ThunderX we used to have a patch (http://www.spinics.net/lists/arm-kernel/msg415457.html), which never made it upstream, because vGIC fixes stopped requiring it at some point. And also we may want to use the nicvf driver not only on actual hardware, but also inside virtual machine in KVM. So do we need to set CONFIG_FORCE_MAX_ZONEORDER for virt too? And what if at some point qemu emulates not only "virt", but some other machine (let's say AMD X-Gene), and we run it on ThunderX and want to use nicvf with this model?
b) IMHO it's not good to have a driver which simply does not work without some obscure option in boot arguments.

 So, i see several possible ways to solve this:

1. Introduce some mechanism which would allow the driver to tell the kernel that it needs coherent pool of large size. Can be problematic because the driver can be a module, and pool allocation happens early.
2. Can we use some other method for allocating queues, which would not require such a huge coherent pool?
3. The driver could check value of atomic_pool_size and adjust own memory requirements accordingly. This indeed looks like a quick hack, but would at least make things running quickly.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Pavel Fedin]

 Hello!

> > Probably you might have to set "coherent_pool" size in bootargs to a
> > higher value.
> > Can you please check.
> 
>  I have tried to do this. I was able to enlarge the pool up to 4MB, and still got allocation
> failures. At 8MB pool preallocation stops working:
> --- cut ---
> Call trace:
> [<ffffffc00012ddb8>] __alloc_pages_nodemask+0x4f4/0x7d4
> [<ffffffc0007be370>] atomic_pool_init+0x60/0x1a4
> [<ffffffc0007be4d4>] arm64_dma_init+0x20/0x28
> [<ffffffc000082848>] do_one_initcall+0x8c/0x1a4
> [<ffffffc0007baac0>] kernel_init_freeable+0x154/0x1f4
> [<ffffffc0005c2b14>] kernel_init+0x10/0xd8
> DMA: failed to allocate 8192 KiB pool for atomic coherent allocation
> --- cut ---
>  and i get even worse faults in the driver.
> 
>  I know that it is possible to allocate larger pools by setting CONFIG_FORCE_MAX_ZONEORDER,
> but:
> a) This is done on per-platform basis. For ThunderX we used to have a patch
> (http://www.spinics.net/lists/arm-kernel/msg415457.html), which never made it upstream,
> because vGIC fixes stopped requiring it at some point. And also we may want to use the nicvf
> driver not only on actual hardware, but also inside virtual machine in KVM. So do we need to
> set CONFIG_FORCE_MAX_ZONEORDER for virt too? And what if at some point qemu emulates not only
> "virt", but some other machine (let's say AMD X-Gene), and we run it on ThunderX and want to
> use nicvf with this model?
> b) IMHO it's not good to have a driver which simply does not work without some obscure option
> in boot arguments.
> 
>  So, i see several possible ways to solve this:
> 
> 1. Introduce some mechanism which would allow the driver to tell the kernel that it needs
> coherent pool of large size. Can be problematic because the driver can be a module, and pool
> allocation happens early.
> 2. Can we use some other method for allocating queues, which would not require such a huge
> coherent pool?
> 3. The driver could check value of atomic_pool_size and adjust own memory requirements
> accordingly. This indeed looks like a quick hack, but would at least make things running
> quickly.

 I have also noticed that CONFIG_DMA_CMA is turned off in my kernel. I guess it was a leftover from old defconfig, because i carry over my .config from version to version. I enabled it and rebuilt the kernel, but in order to get the driver working with this patch i had to also add cma=32M option to kernel arguments. With default of 16M the allocation still fails.
 Should we add Kconfig dependencies?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Pavel Fedin]

 Hello!

> >  So, i see several possible ways to solve this:
> >
> > 1. Introduce some mechanism which would allow the driver to tell the kernel that it needs
> > coherent pool of large size. Can be problematic because the driver can be a module, and pool
> > allocation happens early.
> > 2. Can we use some other method for allocating queues, which would not require such a huge
> > coherent pool?
> > 3. The driver could check value of atomic_pool_size and adjust own memory requirements
> > accordingly. This indeed looks like a quick hack, but would at least make things running
> > quickly.
> 
>  I have also noticed that CONFIG_DMA_CMA is turned off in my kernel. I guess it was a leftover
> from old defconfig, because i carry over my .config from version to version. I enabled it and
> rebuilt the kernel, but in order to get the driver working with this patch i had to also add
> cma=32M option to kernel arguments. With default of 16M the allocation still fails.
>  Should we add Kconfig dependencies?

 After getting it working in guest i tried to apply it to host. With total of 128 virtual functions (= 128 interfaces) it does not work at all. Even after bumping cma region size to insane value of 2GB more than half of interfaces still failed to allocate queues. And after setting cma=3G i could not mount my rootfs.
 So, absolute NAK, unfortunately.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Sunil Kovvuri]

>After getting it working in guest i tried to apply it to host. With total of 128 virtual functions (= 128 interfaces) it does not work at all.
> Even after bumping cma region size to insane value of 2GB more than half of interfaces still failed to allocate queues.
> And after setting cma=3G i could not mount my rootfs.

Here what you are saying is half of the interfaces were initialized
succesfully and rest didn't.
So this issue is not something which is introduced by this patch.

> Can we use some other method for allocating queues, which would not require such a huge coherent pool?
There are so many drivers which use dma_alloc_coherent() directly or
via pci_alloc_consistent() to
allocate memory. How many drivers should we modify.

RE: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Pavel Fedin]

 Hello!

> >After getting it working in guest i tried to apply it to host. With total of 128 virtual
> functions (= 128 interfaces) it does not work at all.
> > Even after bumping cma region size to insane value of 2GB more than half of interfaces still
> failed to allocate queues.
> > And after setting cma=3G i could not mount my rootfs.
> 
> Here what you are saying is half of the interfaces were initialized
> succesfully and rest didn't.

 After setting cma=2G. With default setting of 16M none of them initialized.

> So this issue is not something which is introduced by this patch.

 Before this patch all my interfaces were working.
 I would say the problem with your patch is that it introduces memory requirements which cannot be satisfied by the platform. It's combination of several factors which stops the thing from working, not a single factor. Using dma_alloc_coherent() is not all wrong by itself, of course.
 Perhaps you did some tricks with your configuration, which make it working. Then, i guess, you should have at least described them in commit message of your patch. Or describe all dependencies in KConfig of your driver, which is better. Or, if the platform needs some very special defconfig, add it to arch/arm64/configs (however, i guess, the goal of ARM64 Linux is to run on all possible hardware, so this would not be good from maintainers' POV).

 Sorry, but this is all i can say. In previous messages i have already suggested several ways to solve the problem (too lazy to quite here, 4 IIRC), or you can suggest your own one and let us test it, or you can even stick to "It works for me, i am the only right guy in the world, and i don't care if it doesn't work for you" position and let David decide who of us is right (and he already did that once).
 Basically, here is what i did: i took kernel 4.2, added ThunderX PCI drivers to it (they were posted but NAKed those days back, there's some lazy progress on them currently), added necessary errata patches (also posted on lists, all merged into 4.4), took defconfig, adjusted it according to my needs, and this is what i'm running on my board and this is what i'm using for development. If you point me at what i'm doing wrong way, i'll be glad to accept this.
 I'm over.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH 3/6] net: thunderx: Increase transmit queue length

[Pavel Fedin]

 Hello!

> > swiotlb: coherent allocation failed for device 0002:01:08.4 size=4198400
> > CPU: 2 PID: 3655 Comm: NetworkManager Tainted: G        W  O    4.2.6+ #201
> > Hardware name: Cavium ThunderX CN88XX
> 
> Are you sure 4.2.6 kernel is suitable for backporting this patch aimed
> for linux-4.5 ?

 Why not? It's just a networking driver. And, i also have the same problem on 4.3 running as KVM guest with VFIO.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

[PATCH 5/6] net: thunderx: Switchon carrier only upon interface link up

[Sunil Goutham]

From: Sunil Goutham <sgoutham@cavium.com>

Call netif_carrier_on() only if interface's link is up. Switching this on
upon IFF_UP by default, is causing issues with ethernet channel bonding
in LACP mode. Initial NETDEV_CHANGE notification was being skipped.

Also fixed some issues with link/speed/duplex reporting via ethtool.

Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
---
 .../net/ethernet/cavium/thunder/nicvf_ethtool.c    |   16 +++++++++++++++-
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   |    4 +---
 drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |    2 ++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
index af54c10..a12b2e3 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
@@ -112,6 +112,13 @@ static int nicvf_get_settings(struct net_device *netdev,
 
 	cmd->supported = 0;
 	cmd->transceiver = XCVR_EXTERNAL;
+
+	if (!nic->link_up) {
+		cmd->duplex = DUPLEX_UNKNOWN;
+		ethtool_cmd_speed_set(cmd, SPEED_UNKNOWN);
+		return 0;
+	}
+
 	if (nic->speed <= 1000) {
 		cmd->port = PORT_MII;
 		cmd->autoneg = AUTONEG_ENABLE;
@@ -125,6 +132,13 @@ static int nicvf_get_settings(struct net_device *netdev,
 	return 0;
 }
 
+static u32 nicvf_get_link(struct net_device *netdev)
+{
+	struct nicvf *nic = netdev_priv(netdev);
+
+	return nic->link_up;
+}
+
 static void nicvf_get_drvinfo(struct net_device *netdev,
 			      struct ethtool_drvinfo *info)
 {
@@ -660,7 +674,7 @@ static int nicvf_set_channels(struct net_device *dev,
 
 static const struct ethtool_ops nicvf_ethtool_ops = {
 	.get_settings		= nicvf_get_settings,
-	.get_link		= ethtool_op_get_link,
+	.get_link		= nicvf_get_link,
 	.get_drvinfo		= nicvf_get_drvinfo,
 	.get_msglevel		= nicvf_get_msglevel,
 	.set_msglevel		= nicvf_set_msglevel,
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 7f709cb..dde8dc7 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1057,6 +1057,7 @@ int nicvf_stop(struct net_device *netdev)
 
 	netif_carrier_off(netdev);
 	netif_tx_stop_all_queues(nic->netdev);
+	nic->link_up = false;
 
 	/* Teardown secondary qsets first */
 	if (!nic->sqs_mode) {
@@ -1211,9 +1212,6 @@ int nicvf_open(struct net_device *netdev)
 	nic->drv_stats.txq_stop = 0;
 	nic->drv_stats.txq_wake = 0;
 
-	netif_carrier_on(netdev);
-	netif_tx_start_all_queues(netdev);
-
 	return 0;
 cleanup:
 	nicvf_disable_intr(nic, NICVF_INTR_MBOX, 0);
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index 2076ac3..d9f27ad 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -612,6 +612,8 @@ static void bgx_poll_for_link(struct work_struct *work)
 		lmac->last_duplex = 1;
 	} else {
 		lmac->link_up = 0;
+		lmac->last_speed = SPEED_UNKNOWN;
+		lmac->last_duplex = DUPLEX_UNKNOWN;
 	}
 
 	if (lmac->last_link != lmac->link_up) {
-- 
1.7.1

RE: [PATCH 5/6] net: thunderx: Switchon carrier only upon interface link up

[Pavel Fedin]

 Hello!

 This one causes the network to stop working on Fedora 21. Probably has to do with NetworkManager, which sees something unexpected.
IP address is never set up and connection is never activated, despite it has UP flag.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Tuesday, December 01, 2015 12:14 PM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
> Subject: [PATCH 5/6] net: thunderx: Switchon carrier only upon interface link up
> 
> From: Sunil Goutham <sgoutham@cavium.com>
> 
> Call netif_carrier_on() only if interface's link is up. Switching this on
> upon IFF_UP by default, is causing issues with ethernet channel bonding
> in LACP mode. Initial NETDEV_CHANGE notification was being skipped.
> 
> Also fixed some issues with link/speed/duplex reporting via ethtool.
> 
> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> ---
>  .../net/ethernet/cavium/thunder/nicvf_ethtool.c    |   16 +++++++++++++++-
>  drivers/net/ethernet/cavium/thunder/nicvf_main.c   |    4 +---
>  drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |    2 ++
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
> b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
> index af54c10..a12b2e3 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
> @@ -112,6 +112,13 @@ static int nicvf_get_settings(struct net_device *netdev,
> 
>  	cmd->supported = 0;
>  	cmd->transceiver = XCVR_EXTERNAL;
> +
> +	if (!nic->link_up) {
> +		cmd->duplex = DUPLEX_UNKNOWN;
> +		ethtool_cmd_speed_set(cmd, SPEED_UNKNOWN);
> +		return 0;
> +	}
> +
>  	if (nic->speed <= 1000) {
>  		cmd->port = PORT_MII;
>  		cmd->autoneg = AUTONEG_ENABLE;
> @@ -125,6 +132,13 @@ static int nicvf_get_settings(struct net_device *netdev,
>  	return 0;
>  }
> 
> +static u32 nicvf_get_link(struct net_device *netdev)
> +{
> +	struct nicvf *nic = netdev_priv(netdev);
> +
> +	return nic->link_up;
> +}
> +
>  static void nicvf_get_drvinfo(struct net_device *netdev,
>  			      struct ethtool_drvinfo *info)
>  {
> @@ -660,7 +674,7 @@ static int nicvf_set_channels(struct net_device *dev,
> 
>  static const struct ethtool_ops nicvf_ethtool_ops = {
>  	.get_settings		= nicvf_get_settings,
> -	.get_link		= ethtool_op_get_link,
> +	.get_link		= nicvf_get_link,
>  	.get_drvinfo		= nicvf_get_drvinfo,
>  	.get_msglevel		= nicvf_get_msglevel,
>  	.set_msglevel		= nicvf_set_msglevel,
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index 7f709cb..dde8dc7 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1057,6 +1057,7 @@ int nicvf_stop(struct net_device *netdev)
> 
>  	netif_carrier_off(netdev);
>  	netif_tx_stop_all_queues(nic->netdev);
> +	nic->link_up = false;
> 
>  	/* Teardown secondary qsets first */
>  	if (!nic->sqs_mode) {
> @@ -1211,9 +1212,6 @@ int nicvf_open(struct net_device *netdev)
>  	nic->drv_stats.txq_stop = 0;
>  	nic->drv_stats.txq_wake = 0;
> 
> -	netif_carrier_on(netdev);
> -	netif_tx_start_all_queues(netdev);
> -
>  	return 0;
>  cleanup:
>  	nicvf_disable_intr(nic, NICVF_INTR_MBOX, 0);
> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> index 2076ac3..d9f27ad 100644
> --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> @@ -612,6 +612,8 @@ static void bgx_poll_for_link(struct work_struct *work)
>  		lmac->last_duplex = 1;
>  	} else {
>  		lmac->link_up = 0;
> +		lmac->last_speed = SPEED_UNKNOWN;
> +		lmac->last_duplex = DUPLEX_UNKNOWN;
>  	}
> 
>  	if (lmac->last_link != lmac->link_up) {
> --
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] net: thunderx: Switchon carrier only upon interface link up

[Sunil Kovvuri]

Hi Pavel Fedin,

Are running Fedora 21 on Cavium ThunderX ?
Do you see linkup notification (dmesg) ?

If you see the existing driver (pasted snippet below), it does call
netif_carrier_on() upon receiving l
ink up notification from BGX driver.

===
case NIC_MBOX_MSG_BGX_LINK_CHANGE:
        .........
       if (nic->link_up) {
       netdev_info(nic->netdev, "%s: Link is Up %d Mbps %s\n",
                  nic->netdev->name, nic->speed,
                 nic->duplex == DUPLEX_FULL ?
                 "Full duplex" : "Half duplex");
       netif_carrier_on(nic->netdev);
       netif_tx_start_all_queues(nic->netdev);
========

This patch removes calling carrier on by default.


Thanks,
Sunil.

On Tue, Dec 1, 2015 at 9:02 PM, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>  This one causes the network to stop working on Fedora 21. Probably has to do with NetworkManager, which sees something unexpected.
> IP address is never set up and connection is never activated, despite it has UP flag.
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
>> Goutham
>> Sent: Tuesday, December 01, 2015 12:14 PM
>> To: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
>> Subject: [PATCH 5/6] net: thunderx: Switchon carrier only upon interface link up
>>
>> From: Sunil Goutham <sgoutham@cavium.com>
>>
>> Call netif_carrier_on() only if interface's link is up. Switching this on
>> upon IFF_UP by default, is causing issues with ethernet channel bonding
>> in LACP mode. Initial NETDEV_CHANGE notification was being skipped.
>>
>> Also fixed some issues with link/speed/duplex reporting via ethtool.
>>
>> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
>> ---
>>  .../net/ethernet/cavium/thunder/nicvf_ethtool.c    |   16 +++++++++++++++-
>>  drivers/net/ethernet/cavium/thunder/nicvf_main.c   |    4 +---
>>  drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |    2 ++
>>  3 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
>> b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
>> index af54c10..a12b2e3 100644
>> --- a/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
>> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_ethtool.c
>> @@ -112,6 +112,13 @@ static int nicvf_get_settings(struct net_device *netdev,
>>
>>       cmd->supported = 0;
>>       cmd->transceiver = XCVR_EXTERNAL;
>> +
>> +     if (!nic->link_up) {
>> +             cmd->duplex = DUPLEX_UNKNOWN;
>> +             ethtool_cmd_speed_set(cmd, SPEED_UNKNOWN);
>> +             return 0;
>> +     }
>> +
>>       if (nic->speed <= 1000) {
>>               cmd->port = PORT_MII;
>>               cmd->autoneg = AUTONEG_ENABLE;
>> @@ -125,6 +132,13 @@ static int nicvf_get_settings(struct net_device *netdev,
>>       return 0;
>>  }
>>
>> +static u32 nicvf_get_link(struct net_device *netdev)
>> +{
>> +     struct nicvf *nic = netdev_priv(netdev);
>> +
>> +     return nic->link_up;
>> +}
>> +
>>  static void nicvf_get_drvinfo(struct net_device *netdev,
>>                             struct ethtool_drvinfo *info)
>>  {
>> @@ -660,7 +674,7 @@ static int nicvf_set_channels(struct net_device *dev,
>>
>>  static const struct ethtool_ops nicvf_ethtool_ops = {
>>       .get_settings           = nicvf_get_settings,
>> -     .get_link               = ethtool_op_get_link,
>> +     .get_link               = nicvf_get_link,
>>       .get_drvinfo            = nicvf_get_drvinfo,
>>       .get_msglevel           = nicvf_get_msglevel,
>>       .set_msglevel           = nicvf_set_msglevel,
>> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>> b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>> index 7f709cb..dde8dc7 100644
>> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
>> @@ -1057,6 +1057,7 @@ int nicvf_stop(struct net_device *netdev)
>>
>>       netif_carrier_off(netdev);
>>       netif_tx_stop_all_queues(nic->netdev);
>> +     nic->link_up = false;
>>
>>       /* Teardown secondary qsets first */
>>       if (!nic->sqs_mode) {
>> @@ -1211,9 +1212,6 @@ int nicvf_open(struct net_device *netdev)
>>       nic->drv_stats.txq_stop = 0;
>>       nic->drv_stats.txq_wake = 0;
>>
>> -     netif_carrier_on(netdev);
>> -     netif_tx_start_all_queues(netdev);
>> -
>>       return 0;
>>  cleanup:
>>       nicvf_disable_intr(nic, NICVF_INTR_MBOX, 0);
>> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> index 2076ac3..d9f27ad 100644
>> --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> @@ -612,6 +612,8 @@ static void bgx_poll_for_link(struct work_struct *work)
>>               lmac->last_duplex = 1;
>>       } else {
>>               lmac->link_up = 0;
>> +             lmac->last_speed = SPEED_UNKNOWN;
>> +             lmac->last_duplex = DUPLEX_UNKNOWN;
>>       }
>>
>>       if (lmac->last_link != lmac->link_up) {
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 0/2] net: thunderx: Miscellaneous cleanups

[Sunil Goutham]

From: Sunil Goutham <sgoutham@cavium.com>

This patch series contains contains couple of cleanup patches.

Sunil Goutham (1):
  net, thunderx: Remove unnecessary rcv buffer start address management

Yury Norov (1):
  net: thunderx: nicvf_queues: nivc_*_intr: remove duplication

 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |  189 +++++---------------
 drivers/net/ethernet/cavium/thunder/nicvf_queues.h |   10 +-
 2 files changed, 51 insertions(+), 148 deletions(-)

RE: [PATCH 0/2] net: thunderx: Miscellaneous cleanups

[Pavel Fedin]

 Tested-by: Pavel Fedin <p.fedin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Monday, December 07, 2015 8:01 AM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; p.fedin@samsung.com;
> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
> Subject: [PATCH 0/2] net: thunderx: Miscellaneous cleanups
> 
> From: Sunil Goutham <sgoutham@cavium.com>
> 
> This patch series contains contains couple of cleanup patches.
> 
> Sunil Goutham (1):
>   net, thunderx: Remove unnecessary rcv buffer start address management
> 
> Yury Norov (1):
>   net: thunderx: nicvf_queues: nivc_*_intr: remove duplication
> 
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.c |  189 +++++---------------
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.h |   10 +-
>  2 files changed, 51 insertions(+), 148 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] net: thunderx: Miscellaneous cleanups

[David Miller]

From: Sunil Goutham <sunil.kovvuri@gmail.com>
Date: Mon,  7 Dec 2015 10:30:31 +0530

> This patch series contains contains couple of cleanup patches.

Series applied to net-next, thanks.

[PATCH 1/2] net: thunderx: HW TSO support for pass-2 hardware

[Sunil Goutham]

From: Sunil Goutham <sgoutham@cavium.com>

This adds support for offloading TCP segmentation to HW in pass-2
revision of hardware. Both driver level SW TSO for pass1.x chips
and HW TSO for pass-2 chip will co-exist.

Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nic.h          |   12 ++++++--
 drivers/net/ethernet/cavium/thunder/nic_main.c     |   11 ++-----
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   |   15 ++++++++-
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |   20 ++++++++++---
 drivers/net/ethernet/cavium/thunder/q_struct.h     |   30 ++++++++++---------
 5 files changed, 56 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 39ca674..02571f4 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -262,9 +262,10 @@ struct nicvf {
 	struct pci_dev		*pdev;
 	u8			vf_id;
 	u8			node;
-	u8			tns_mode:1;
-	u8			sqs_mode:1;
-	u8			loopback_supported:1;
+	bool			tns_mode:1;
+	bool			sqs_mode:1;
+	bool			loopback_supported:1;
+	bool			hw_tso:1;
 	u16			mtu;
 	struct queue_set	*qs;
 #define	MAX_SQS_PER_VF_SINGLE_NODE		5
@@ -489,6 +490,11 @@ static inline int nic_get_node_id(struct pci_dev *pdev)
 	return ((addr >> NIC_NODE_ID_SHIFT) & NIC_NODE_ID_MASK);
 }
 
+static inline bool pass1_silicon(struct pci_dev *pdev)
+{
+	return pdev->revision < 8;
+}
+
 int nicvf_set_real_num_queues(struct net_device *netdev,
 			      int tx_queues, int rx_queues);
 int nicvf_open(struct net_device *netdev);
diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c b/drivers/net/ethernet/cavium/thunder/nic_main.c
index 4b7fd63..9f80de4 100644
--- a/drivers/net/ethernet/cavium/thunder/nic_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
@@ -55,11 +55,6 @@ struct nicpf {
 	bool			irq_allocated[NIC_PF_MSIX_VECTORS];
 };
 
-static inline bool pass1_silicon(struct nicpf *nic)
-{
-	return nic->pdev->revision < 8;
-}
-
 /* Supported devices */
 static const struct pci_device_id nic_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDER_NIC_PF) },
@@ -123,7 +118,7 @@ static void nic_send_msg_to_vf(struct nicpf *nic, int vf, union nic_mbx *mbx)
 	 * when PF writes to MBOX(1), in next revisions when
 	 * PF writes to MBOX(0)
 	 */
-	if (pass1_silicon(nic)) {
+	if (pass1_silicon(nic->pdev)) {
 		/* see the comment for nic_reg_write()/nic_reg_read()
 		 * functions above
 		 */
@@ -400,7 +395,7 @@ static void nic_config_cpi(struct nicpf *nic, struct cpi_cfg_msg *cfg)
 			padd = cpi % 8; /* 3 bits CS out of 6bits DSCP */
 
 		/* Leave RSS_SIZE as '0' to disable RSS */
-		if (pass1_silicon(nic)) {
+		if (pass1_silicon(nic->pdev)) {
 			nic_reg_write(nic, NIC_PF_CPI_0_2047_CFG | (cpi << 3),
 				      (vnic << 24) | (padd << 16) |
 				      (rssi_base + rssi));
@@ -470,7 +465,7 @@ static void nic_config_rss(struct nicpf *nic, struct rss_cfg_msg *cfg)
 	}
 
 	cpi_base = nic->cpi_base[cfg->vf_id];
-	if (pass1_silicon(nic))
+	if (pass1_silicon(nic->pdev))
 		idx_addr = NIC_PF_CPI_0_2047_CFG;
 	else
 		idx_addr = NIC_PF_MPI_0_2047_CFG;
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index dde8dc7..c24cb2a 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -525,14 +525,22 @@ static void nicvf_snd_pkt_handler(struct net_device *netdev,
 		   __func__, cqe_tx->sq_qs, cqe_tx->sq_idx,
 		   cqe_tx->sqe_ptr, hdr->subdesc_cnt);
 
-	nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
 	nicvf_check_cqe_tx_errs(nic, cq, cqe_tx);
 	skb = (struct sk_buff *)sq->skbuff[cqe_tx->sqe_ptr];
-	/* For TSO offloaded packets only one head SKB needs to be freed */
+	/* For TSO offloaded packets only one SQE will have a valid SKB */
 	if (skb) {
+		nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
 		prefetch(skb);
 		dev_consume_skb_any(skb);
 		sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
+	} else {
+		/* In case of HW TSO, HW sends a CQE for each segment of a TSO
+		 * packet instead of a single CQE for the whole TSO packet
+		 * transmitted. Each of this CQE points to the same SQE, so
+		 * avoid freeing same SQE multiple times.
+		 */
+		if (!nic->hw_tso)
+			nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
 	}
 }
 
@@ -1549,6 +1557,9 @@ static int nicvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	netdev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
 
+	if (!pass1_silicon(nic->pdev))
+		nic->hw_tso = true;
+
 	netdev->netdev_ops = &nicvf_netdev_ops;
 	netdev->watchdog_timeo = NICVF_TX_TIMEOUT;
 
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
index 1fbd908..b11fc09 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
@@ -925,7 +925,7 @@ static int nicvf_sq_subdesc_required(struct nicvf *nic, struct sk_buff *skb)
 {
 	int subdesc_cnt = MIN_SQ_DESC_PER_PKT_XMIT;
 
-	if (skb_shinfo(skb)->gso_size) {
+	if (skb_shinfo(skb)->gso_size && !nic->hw_tso) {
 		subdesc_cnt = nicvf_tso_count_subdescs(skb);
 		return subdesc_cnt;
 	}
@@ -940,7 +940,7 @@ static int nicvf_sq_subdesc_required(struct nicvf *nic, struct sk_buff *skb)
  * First subdescriptor for every send descriptor.
  */
 static inline void
-nicvf_sq_add_hdr_subdesc(struct snd_queue *sq, int qentry,
+nicvf_sq_add_hdr_subdesc(struct nicvf *nic, struct snd_queue *sq, int qentry,
 			 int subdesc_cnt, struct sk_buff *skb, int len)
 {
 	int proto;
@@ -976,6 +976,15 @@ nicvf_sq_add_hdr_subdesc(struct snd_queue *sq, int qentry,
 			break;
 		}
 	}
+
+	if (nic->hw_tso && skb_shinfo(skb)->gso_size) {
+		hdr->tso = 1;
+		hdr->tso_start = skb_transport_offset(skb) + tcp_hdrlen(skb);
+		hdr->tso_max_paysize = skb_shinfo(skb)->gso_size;
+		/* For non-tunneled pkts, point this to L2 ethertype */
+		hdr->inner_l3_offset = skb_network_offset(skb) - 2;
+		nic->drv_stats.tx_tso++;
+	}
 }
 
 /* SQ GATHER subdescriptor
@@ -1045,7 +1054,7 @@ static int nicvf_sq_append_tso(struct nicvf *nic, struct snd_queue *sq,
 			data_left -= size;
 			tso_build_data(skb, &tso, size);
 		}
-		nicvf_sq_add_hdr_subdesc(sq, hdr_qentry,
+		nicvf_sq_add_hdr_subdesc(nic, sq, hdr_qentry,
 					 seg_subdescs - 1, skb, seg_len);
 		sq->skbuff[hdr_qentry] = (u64)NULL;
 		qentry = nicvf_get_nxt_sqentry(sq, qentry);
@@ -1098,11 +1107,12 @@ int nicvf_sq_append_skb(struct nicvf *nic, struct sk_buff *skb)
 	qentry = nicvf_get_sq_desc(sq, subdesc_cnt);
 
 	/* Check if its a TSO packet */
-	if (skb_shinfo(skb)->gso_size)
+	if (skb_shinfo(skb)->gso_size && !nic->hw_tso)
 		return nicvf_sq_append_tso(nic, sq, sq_num, qentry, skb);
 
 	/* Add SQ header subdesc */
-	nicvf_sq_add_hdr_subdesc(sq, qentry, subdesc_cnt - 1, skb, skb->len);
+	nicvf_sq_add_hdr_subdesc(nic, sq, qentry, subdesc_cnt - 1,
+				 skb, skb->len);
 
 	/* Add SQ gather subdescs */
 	qentry = nicvf_get_nxt_sqentry(sq, qentry);
diff --git a/drivers/net/ethernet/cavium/thunder/q_struct.h b/drivers/net/ethernet/cavium/thunder/q_struct.h
index 3c1de97..9e6d987 100644
--- a/drivers/net/ethernet/cavium/thunder/q_struct.h
+++ b/drivers/net/ethernet/cavium/thunder/q_struct.h
@@ -545,25 +545,28 @@ struct sq_hdr_subdesc {
 	u64    subdesc_cnt:8;
 	u64    csum_l4:2;
 	u64    csum_l3:1;
-	u64    rsvd0:5;
+	u64    csum_inner_l4:2;
+	u64    csum_inner_l3:1;
+	u64    rsvd0:2;
 	u64    l4_offset:8;
 	u64    l3_offset:8;
 	u64    rsvd1:4;
 	u64    tot_len:20; /* W0 */
 
-	u64    tso_sdc_cont:8;
-	u64    tso_sdc_first:8;
-	u64    tso_l4_offset:8;
-	u64    tso_flags_last:12;
-	u64    tso_flags_first:12;
-	u64    rsvd2:2;
+	u64    rsvd2:24;
+	u64    inner_l4_offset:8;
+	u64    inner_l3_offset:8;
+	u64    tso_start:8;
+	u64    rsvd3:2;
 	u64    tso_max_paysize:14; /* W1 */
 #elif defined(__LITTLE_ENDIAN_BITFIELD)
 	u64    tot_len:20;
 	u64    rsvd1:4;
 	u64    l3_offset:8;
 	u64    l4_offset:8;
-	u64    rsvd0:5;
+	u64    rsvd0:2;
+	u64    csum_inner_l3:1;
+	u64    csum_inner_l4:2;
 	u64    csum_l3:1;
 	u64    csum_l4:2;
 	u64    subdesc_cnt:8;
@@ -574,12 +577,11 @@ struct sq_hdr_subdesc {
 	u64    subdesc_type:4; /* W0 */
 
 	u64    tso_max_paysize:14;
-	u64    rsvd2:2;
-	u64    tso_flags_first:12;
-	u64    tso_flags_last:12;
-	u64    tso_l4_offset:8;
-	u64    tso_sdc_first:8;
-	u64    tso_sdc_cont:8; /* W1 */
+	u64    rsvd3:2;
+	u64    tso_start:8;
+	u64    inner_l3_offset:8;
+	u64    inner_l4_offset:8;
+	u64    rsvd2:24; /* W1 */
 #endif
 };
 
-- 
1.7.1

RE: [PATCH 1/2] net: thunderx: HW TSO support for pass-2 hardware

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Wednesday, December 09, 2015 2:38 PM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; p.fedin@samsung.com;
> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
> Subject: [PATCH 1/2] net: thunderx: HW TSO support for pass-2 hardware
> 
> From: Sunil Goutham <sgoutham@cavium.com>
> 
> This adds support for offloading TCP segmentation to HW in pass-2
> revision of hardware. Both driver level SW TSO for pass1.x chips
> and HW TSO for pass-2 chip will co-exist.
> 
> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> ---
>  drivers/net/ethernet/cavium/thunder/nic.h          |   12 ++++++--
>  drivers/net/ethernet/cavium/thunder/nic_main.c     |   11 ++-----
>  drivers/net/ethernet/cavium/thunder/nicvf_main.c   |   15 ++++++++-
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.c |   20 ++++++++++---
>  drivers/net/ethernet/cavium/thunder/q_struct.h     |   30 ++++++++++---------
>  5 files changed, 56 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h
> b/drivers/net/ethernet/cavium/thunder/nic.h
> index 39ca674..02571f4 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -262,9 +262,10 @@ struct nicvf {
>  	struct pci_dev		*pdev;
>  	u8			vf_id;
>  	u8			node;
> -	u8			tns_mode:1;
> -	u8			sqs_mode:1;
> -	u8			loopback_supported:1;
> +	bool			tns_mode:1;
> +	bool			sqs_mode:1;

 These little refactors are creeping in your code without even being mentioned in the commit message, this is not good practice
IMHO. Additionally, may be turn these two flags into something like:

enum nicvf_mode {
	NICVF_BYPASS,
	NICVF_TNS,
	NICVF_SQS
};

 ? Anyway, these modes are mutually exclusive.

> +	bool			loopback_supported:1;
> +	bool			hw_tso:1;
>  	u16			mtu;
>  	struct queue_set	*qs;
>  #define	MAX_SQS_PER_VF_SINGLE_NODE		5
> @@ -489,6 +490,11 @@ static inline int nic_get_node_id(struct pci_dev *pdev)
>  	return ((addr >> NIC_NODE_ID_SHIFT) & NIC_NODE_ID_MASK);
>  }
> 
> +static inline bool pass1_silicon(struct pci_dev *pdev)
> +{
> +	return pdev->revision < 8;
> +}
> +
>  int nicvf_set_real_num_queues(struct net_device *netdev,
>  			      int tx_queues, int rx_queues);
>  int nicvf_open(struct net_device *netdev);
> diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c
> b/drivers/net/ethernet/cavium/thunder/nic_main.c
> index 4b7fd63..9f80de4 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
> @@ -55,11 +55,6 @@ struct nicpf {
>  	bool			irq_allocated[NIC_PF_MSIX_VECTORS];
>  };
> 
> -static inline bool pass1_silicon(struct nicpf *nic)
> -{
> -	return nic->pdev->revision < 8;
> -}
> -
>  /* Supported devices */
>  static const struct pci_device_id nic_id_table[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDER_NIC_PF) },
> @@ -123,7 +118,7 @@ static void nic_send_msg_to_vf(struct nicpf *nic, int vf, union nic_mbx
> *mbx)
>  	 * when PF writes to MBOX(1), in next revisions when
>  	 * PF writes to MBOX(0)
>  	 */
> -	if (pass1_silicon(nic)) {
> +	if (pass1_silicon(nic->pdev)) {
>  		/* see the comment for nic_reg_write()/nic_reg_read()
>  		 * functions above
>  		 */
> @@ -400,7 +395,7 @@ static void nic_config_cpi(struct nicpf *nic, struct cpi_cfg_msg *cfg)
>  			padd = cpi % 8; /* 3 bits CS out of 6bits DSCP */
> 
>  		/* Leave RSS_SIZE as '0' to disable RSS */
> -		if (pass1_silicon(nic)) {
> +		if (pass1_silicon(nic->pdev)) {
>  			nic_reg_write(nic, NIC_PF_CPI_0_2047_CFG | (cpi << 3),
>  				      (vnic << 24) | (padd << 16) |
>  				      (rssi_base + rssi));
> @@ -470,7 +465,7 @@ static void nic_config_rss(struct nicpf *nic, struct rss_cfg_msg *cfg)
>  	}
> 
>  	cpi_base = nic->cpi_base[cfg->vf_id];
> -	if (pass1_silicon(nic))
> +	if (pass1_silicon(nic->pdev))
>  		idx_addr = NIC_PF_CPI_0_2047_CFG;
>  	else
>  		idx_addr = NIC_PF_MPI_0_2047_CFG;
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index dde8dc7..c24cb2a 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -525,14 +525,22 @@ static void nicvf_snd_pkt_handler(struct net_device *netdev,
>  		   __func__, cqe_tx->sq_qs, cqe_tx->sq_idx,
>  		   cqe_tx->sqe_ptr, hdr->subdesc_cnt);
> 
> -	nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
>  	nicvf_check_cqe_tx_errs(nic, cq, cqe_tx);
>  	skb = (struct sk_buff *)sq->skbuff[cqe_tx->sqe_ptr];
> -	/* For TSO offloaded packets only one head SKB needs to be freed */
> +	/* For TSO offloaded packets only one SQE will have a valid SKB */
>  	if (skb) {
> +		nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
>  		prefetch(skb);
>  		dev_consume_skb_any(skb);
>  		sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
> +	} else {
> +		/* In case of HW TSO, HW sends a CQE for each segment of a TSO
> +		 * packet instead of a single CQE for the whole TSO packet
> +		 * transmitted. Each of this CQE points to the same SQE, so
> +		 * avoid freeing same SQE multiple times.
> +		 */
> +		if (!nic->hw_tso)
> +			nicvf_put_sq_desc(sq, hdr->subdesc_cnt + 1);
>  	}
>  }
> 
> @@ -1549,6 +1557,9 @@ static int nicvf_probe(struct pci_dev *pdev, const struct pci_device_id
> *ent)
> 
>  	netdev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
> 
> +	if (!pass1_silicon(nic->pdev))
> +		nic->hw_tso = true;
> +
>  	netdev->netdev_ops = &nicvf_netdev_ops;
>  	netdev->watchdog_timeo = NICVF_TX_TIMEOUT;
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> index 1fbd908..b11fc09 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> @@ -925,7 +925,7 @@ static int nicvf_sq_subdesc_required(struct nicvf *nic, struct sk_buff
> *skb)
>  {
>  	int subdesc_cnt = MIN_SQ_DESC_PER_PKT_XMIT;
> 
> -	if (skb_shinfo(skb)->gso_size) {
> +	if (skb_shinfo(skb)->gso_size && !nic->hw_tso) {
>  		subdesc_cnt = nicvf_tso_count_subdescs(skb);
>  		return subdesc_cnt;
>  	}
> @@ -940,7 +940,7 @@ static int nicvf_sq_subdesc_required(struct nicvf *nic, struct sk_buff
> *skb)
>   * First subdescriptor for every send descriptor.
>   */
>  static inline void
> -nicvf_sq_add_hdr_subdesc(struct snd_queue *sq, int qentry,
> +nicvf_sq_add_hdr_subdesc(struct nicvf *nic, struct snd_queue *sq, int qentry,
>  			 int subdesc_cnt, struct sk_buff *skb, int len)
>  {
>  	int proto;
> @@ -976,6 +976,15 @@ nicvf_sq_add_hdr_subdesc(struct snd_queue *sq, int qentry,
>  			break;
>  		}
>  	}
> +
> +	if (nic->hw_tso && skb_shinfo(skb)->gso_size) {
> +		hdr->tso = 1;
> +		hdr->tso_start = skb_transport_offset(skb) + tcp_hdrlen(skb);
> +		hdr->tso_max_paysize = skb_shinfo(skb)->gso_size;
> +		/* For non-tunneled pkts, point this to L2 ethertype */
> +		hdr->inner_l3_offset = skb_network_offset(skb) - 2;
> +		nic->drv_stats.tx_tso++;
> +	}
>  }
> 
>  /* SQ GATHER subdescriptor
> @@ -1045,7 +1054,7 @@ static int nicvf_sq_append_tso(struct nicvf *nic, struct snd_queue *sq,
>  			data_left -= size;
>  			tso_build_data(skb, &tso, size);
>  		}
> -		nicvf_sq_add_hdr_subdesc(sq, hdr_qentry,
> +		nicvf_sq_add_hdr_subdesc(nic, sq, hdr_qentry,
>  					 seg_subdescs - 1, skb, seg_len);
>  		sq->skbuff[hdr_qentry] = (u64)NULL;
>  		qentry = nicvf_get_nxt_sqentry(sq, qentry);
> @@ -1098,11 +1107,12 @@ int nicvf_sq_append_skb(struct nicvf *nic, struct sk_buff *skb)
>  	qentry = nicvf_get_sq_desc(sq, subdesc_cnt);
> 
>  	/* Check if its a TSO packet */
> -	if (skb_shinfo(skb)->gso_size)
> +	if (skb_shinfo(skb)->gso_size && !nic->hw_tso)
>  		return nicvf_sq_append_tso(nic, sq, sq_num, qentry, skb);
> 
>  	/* Add SQ header subdesc */
> -	nicvf_sq_add_hdr_subdesc(sq, qentry, subdesc_cnt - 1, skb, skb->len);
> +	nicvf_sq_add_hdr_subdesc(nic, sq, qentry, subdesc_cnt - 1,
> +				 skb, skb->len);
> 
>  	/* Add SQ gather subdescs */
>  	qentry = nicvf_get_nxt_sqentry(sq, qentry);
> diff --git a/drivers/net/ethernet/cavium/thunder/q_struct.h
> b/drivers/net/ethernet/cavium/thunder/q_struct.h
> index 3c1de97..9e6d987 100644
> --- a/drivers/net/ethernet/cavium/thunder/q_struct.h
> +++ b/drivers/net/ethernet/cavium/thunder/q_struct.h
> @@ -545,25 +545,28 @@ struct sq_hdr_subdesc {
>  	u64    subdesc_cnt:8;
>  	u64    csum_l4:2;
>  	u64    csum_l3:1;
> -	u64    rsvd0:5;
> +	u64    csum_inner_l4:2;
> +	u64    csum_inner_l3:1;
> +	u64    rsvd0:2;
>  	u64    l4_offset:8;
>  	u64    l3_offset:8;
>  	u64    rsvd1:4;
>  	u64    tot_len:20; /* W0 */
> 
> -	u64    tso_sdc_cont:8;
> -	u64    tso_sdc_first:8;
> -	u64    tso_l4_offset:8;
> -	u64    tso_flags_last:12;
> -	u64    tso_flags_first:12;
> -	u64    rsvd2:2;
> +	u64    rsvd2:24;
> +	u64    inner_l4_offset:8;
> +	u64    inner_l3_offset:8;
> +	u64    tso_start:8;
> +	u64    rsvd3:2;
>  	u64    tso_max_paysize:14; /* W1 */
>  #elif defined(__LITTLE_ENDIAN_BITFIELD)
>  	u64    tot_len:20;
>  	u64    rsvd1:4;
>  	u64    l3_offset:8;
>  	u64    l4_offset:8;
> -	u64    rsvd0:5;
> +	u64    rsvd0:2;
> +	u64    csum_inner_l3:1;
> +	u64    csum_inner_l4:2;
>  	u64    csum_l3:1;
>  	u64    csum_l4:2;
>  	u64    subdesc_cnt:8;
> @@ -574,12 +577,11 @@ struct sq_hdr_subdesc {
>  	u64    subdesc_type:4; /* W0 */
> 
>  	u64    tso_max_paysize:14;
> -	u64    rsvd2:2;
> -	u64    tso_flags_first:12;
> -	u64    tso_flags_last:12;
> -	u64    tso_l4_offset:8;
> -	u64    tso_sdc_first:8;
> -	u64    tso_sdc_cont:8; /* W1 */
> +	u64    rsvd3:2;
> +	u64    tso_start:8;
> +	u64    inner_l3_offset:8;
> +	u64    inner_l4_offset:8;
> +	u64    rsvd2:24; /* W1 */
>  #endif
>  };
> 
> --
> 1.7.1
> 

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/2] net: thunderx: HW TSO support for pass-2 hardware

[Sunil Kovvuri]

>> +     bool                    tns_mode:1;
>> +     bool                    sqs_mode:1;

 >These little refactors are creeping in your code without even being
mentioned in the commit message, this is not good practice.
Okay, will include these in the commit message.

>IMHO. Additionally, may be turn these two flags into something like:

>enum nicvf_mode {
>        NICVF_BYPASS,
>        NICVF_TNS,
>        NICVF_SQS
>};

 >? Anyway, these modes are mutually exclusive.
VF driver always assumed to be in BYPASS mode.
Will submit a seperate cleanup patch to get rid of 'tns_mode' boolean.

Thanks,
Sunil.

Re: [PATCH 1/2] net: thunderx: HW TSO support for pass-2 hardware

[David Miller]

From: Pavel Fedin <p.fedin@samsung.com>
Date: Wed, 09 Dec 2015 15:05:01 +0300

>  Hello!
> 
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
>> Goutham
>> Sent: Wednesday, December 09, 2015 2:38 PM
>> To: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; p.fedin@samsung.com;
>> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
>> Subject: [PATCH 1/2] net: thunderx: HW TSO support for pass-2 hardware
>> 
>> From: Sunil Goutham <sgoutham@cavium.com>
>> 
>> This adds support for offloading TCP segmentation to HW in pass-2
>> revision of hardware. Both driver level SW TSO for pass1.x chips
>> and HW TSO for pass-2 chip will co-exist.
>> 
>> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
>> ---
>>  drivers/net/ethernet/cavium/thunder/nic.h          |   12 ++++++--
>>  drivers/net/ethernet/cavium/thunder/nic_main.c     |   11 ++-----
>>  drivers/net/ethernet/cavium/thunder/nicvf_main.c   |   15 ++++++++-
>>  drivers/net/ethernet/cavium/thunder/nicvf_queues.c |   20 ++++++++++---
>>  drivers/net/ethernet/cavium/thunder/q_struct.h     |   30 ++++++++++---------
>>  5 files changed, 56 insertions(+), 32 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h
>> b/drivers/net/ethernet/cavium/thunder/nic.h
>> index 39ca674..02571f4 100644
>> --- a/drivers/net/ethernet/cavium/thunder/nic.h
>> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
>> @@ -262,9 +262,10 @@ struct nicvf {
>>  	struct pci_dev		*pdev;
>>  	u8			vf_id;
>>  	u8			node;
>> -	u8			tns_mode:1;
>> -	u8			sqs_mode:1;
>> -	u8			loopback_supported:1;
>> +	bool			tns_mode:1;
>> +	bool			sqs_mode:1;
> 
>  These little refactors are creeping in your code without even being mentioned in the commit message, this is not good practice
> IMHO. Additionally, may be turn these two flags into something like:

Also I disagree completely with boolean bitfields.  Just use plain 'bool' and let
the compiler decide how to lay it out.

[PATCH 2/2] net: thunderx: Enable CQE count threshold interrupt

[Sunil Goutham]

From: Sunil Goutham <sgoutham@cavium.com>

This feature is introduced in pass-2 chip and with this CQ interrupt
coalescing will work based on both timer and count.

Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/nic.h          |    2 ++
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   |    2 +-
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |    5 ++++-
 drivers/net/ethernet/cavium/thunder/nicvf_queues.h |    3 ++-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h b/drivers/net/ethernet/cavium/thunder/nic.h
index 02571f4..e782856 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -34,6 +34,8 @@
 /* NIC priv flags */
 #define	NIC_SRIOV_ENABLED		BIT(0)
 
+#define	VNIC_NAPI_WEIGHT		NAPI_POLL_WEIGHT
+
 /* Min/Max packet size */
 #define	NIC_HW_MIN_FRS			64
 #define	NIC_HW_MAX_FRS			9200 /* 9216 max packet including FCS */
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index c24cb2a..e06a7f8 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1155,7 +1155,7 @@ int nicvf_open(struct net_device *netdev)
 		cq_poll->cq_idx = qidx;
 		cq_poll->nicvf = nic;
 		netif_napi_add(netdev, &cq_poll->napi, nicvf_poll,
-			       NAPI_POLL_WEIGHT);
+			       VNIC_NAPI_WEIGHT);
 		napi_enable(&cq_poll->napi);
 		nic->napi[qidx] = cq_poll;
 	}
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
index b11fc09..4e9709e 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
@@ -299,7 +299,10 @@ static int nicvf_init_cmp_queue(struct nicvf *nic,
 		return err;
 
 	cq->desc = cq->dmem.base;
-	cq->thresh = CMP_QUEUE_CQE_THRESH;
+	if (!pass1_silicon(nic->pdev))
+		cq->thresh = CMP_QUEUE_CQE_THRESH;
+	else
+		cq->thresh = 0;
 	nic->cq_coalesce_usecs = (CMP_QUEUE_TIMER_THRESH * 0.05) - 1;
 
 	return 0;
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
index a4f6667..0fae6ad 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
@@ -10,6 +10,7 @@
 #define NICVF_QUEUES_H
 
 #include <linux/netdevice.h>
+#include "nic.h"
 #include "q_struct.h"
 
 #define MAX_QUEUE_SET			128
@@ -75,7 +76,7 @@
  */
 #define CMP_QSIZE		CMP_QUEUE_SIZE2
 #define CMP_QUEUE_LEN		(1ULL << (CMP_QSIZE + 10))
-#define CMP_QUEUE_CQE_THRESH	0
+#define CMP_QUEUE_CQE_THRESH	(VNIC_NAPI_WEIGHT / 2)
 #define CMP_QUEUE_TIMER_THRESH	80 /* ~2usec */
 
 #define RBDR_SIZE		RBDR_SIZE0
-- 
1.7.1

RE: [PATCH 2/2] net: thunderx: Enable CQE count threshold interrupt

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Wednesday, December 09, 2015 2:38 PM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; p.fedin@samsung.com;
> Sunil.Goutham@caviumnetworks.com; Sunil Goutham
> Subject: [PATCH 2/2] net: thunderx: Enable CQE count threshold interrupt
> 
> From: Sunil Goutham <sgoutham@cavium.com>
> 
> This feature is introduced in pass-2 chip and with this CQ interrupt
> coalescing will work based on both timer and count.
> 
> Signed-off-by: Sunil Goutham <sgoutham@cavium.com>
> ---
>  drivers/net/ethernet/cavium/thunder/nic.h          |    2 ++
>  drivers/net/ethernet/cavium/thunder/nicvf_main.c   |    2 +-
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.c |    5 ++++-
>  drivers/net/ethernet/cavium/thunder/nicvf_queues.h |    3 ++-
>  4 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h
> b/drivers/net/ethernet/cavium/thunder/nic.h
> index 02571f4..e782856 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -34,6 +34,8 @@
>  /* NIC priv flags */
>  #define	NIC_SRIOV_ENABLED		BIT(0)
> 
> +#define	VNIC_NAPI_WEIGHT		NAPI_POLL_WEIGHT
> +
>  /* Min/Max packet size */
>  #define	NIC_HW_MIN_FRS			64
>  #define	NIC_HW_MAX_FRS			9200 /* 9216 max packet including FCS */
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> index c24cb2a..e06a7f8 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
> @@ -1155,7 +1155,7 @@ int nicvf_open(struct net_device *netdev)
>  		cq_poll->cq_idx = qidx;
>  		cq_poll->nicvf = nic;
>  		netif_napi_add(netdev, &cq_poll->napi, nicvf_poll,
> -			       NAPI_POLL_WEIGHT);
> +			       VNIC_NAPI_WEIGHT);

 What's the sense in introducing another constant which is aliased to the previous one? Making LOC bigger?

>  		napi_enable(&cq_poll->napi);
>  		nic->napi[qidx] = cq_poll;
>  	}
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> index b11fc09..4e9709e 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.c
> @@ -299,7 +299,10 @@ static int nicvf_init_cmp_queue(struct nicvf *nic,
>  		return err;
> 
>  	cq->desc = cq->dmem.base;
> -	cq->thresh = CMP_QUEUE_CQE_THRESH;
> +	if (!pass1_silicon(nic->pdev))
> +		cq->thresh = CMP_QUEUE_CQE_THRESH;
> +	else
> +		cq->thresh = 0;

 IMHO "cq->thresh = pass1_silicon(nic->pdev) ? CMP_QUEUE_CQE_THRESH : 0" looks less bulky.

>  	nic->cq_coalesce_usecs = (CMP_QUEUE_TIMER_THRESH * 0.05) - 1;
> 
>  	return 0;
> diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> index a4f6667..0fae6ad 100644
> --- a/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> +++ b/drivers/net/ethernet/cavium/thunder/nicvf_queues.h
> @@ -10,6 +10,7 @@
>  #define NICVF_QUEUES_H
> 
>  #include <linux/netdevice.h>
> +#include "nic.h"
>  #include "q_struct.h"
> 
>  #define MAX_QUEUE_SET			128
> @@ -75,7 +76,7 @@
>   */
>  #define CMP_QSIZE		CMP_QUEUE_SIZE2
>  #define CMP_QUEUE_LEN		(1ULL << (CMP_QSIZE + 10))
> -#define CMP_QUEUE_CQE_THRESH	0
> +#define CMP_QUEUE_CQE_THRESH	(VNIC_NAPI_WEIGHT / 2)
>  #define CMP_QUEUE_TIMER_THRESH	80 /* ~2usec */
> 
>  #define RBDR_SIZE		RBDR_SIZE0
> --
> 1.7.1

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 2/2] net: thunderx: Enable CQE count threshold interrupt

[Sunil Kovvuri]

Will take care of above suggestions and resubmit.

Thanks,
Sunil.

[PATCH v2 0/2] net: thunderx: Support for pass-2 hw features

[Sunil Goutham]

From: Sunil Goutham <sgoutham@cavium.com>

This patch set adds support for new features added in pass-2 revision
of hardware like TSO and count based interrupt coalescing.

Changes from v1:
- Addressed comments received regarding boolean bit field changes 
  by excluding them from this patch. Will submit a seperate
  patch along with cleanup of unsed field.
- Got rid of new macro 'VNIC_NAPI_WEIGHT' introduced in 
  count threshold interrupt patch.

Sunil Goutham (2):
  net: thunderx: HW TSO support for pass-2 hardware
  net: thunderx: Enable CQE count threshold interrupt

 drivers/net/ethernet/cavium/thunder/nic.h          |    6 ++++
 drivers/net/ethernet/cavium/thunder/nic_main.c     |   11 ++-----
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   |   15 ++++++++-
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |   22 ++++++++++----
 drivers/net/ethernet/cavium/thunder/nicvf_queues.h |    2 +-
 drivers/net/ethernet/cavium/thunder/q_struct.h     |   30 ++++++++++---------
 6 files changed, 55 insertions(+), 31 deletions(-)

RE: [PATCH v2 0/2] net: thunderx: Support for pass-2 hw features

[Pavel Fedin]

 All series:

 Reviewed-by: Pavel Fedin <p.fedin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Sunil Goutham
Sent: Thursday, December 10, 2015 10:55 AM
To: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; p.fedin@samsung.com; Sunil.Goutham@caviumnetworks.com; Sunil
Goutham
Subject: [PATCH v2 0/2] net: thunderx: Support for pass-2 hw features

From: Sunil Goutham <sgoutham@cavium.com>

This patch set adds support for new features added in pass-2 revision of hardware like TSO and count based interrupt coalescing.

Changes from v1:
- Addressed comments received regarding boolean bit field changes
  by excluding them from this patch. Will submit a seperate
  patch along with cleanup of unsed field.
- Got rid of new macro 'VNIC_NAPI_WEIGHT' introduced in
  count threshold interrupt patch.

Sunil Goutham (2):
  net: thunderx: HW TSO support for pass-2 hardware
  net: thunderx: Enable CQE count threshold interrupt

 drivers/net/ethernet/cavium/thunder/nic.h          |    6 ++++
 drivers/net/ethernet/cavium/thunder/nic_main.c     |   11 ++-----
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   |   15 ++++++++-
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |   22 ++++++++++----
 drivers/net/ethernet/cavium/thunder/nicvf_queues.h |    2 +-
 drivers/net/ethernet/cavium/thunder/q_struct.h     |   30 ++++++++++---------
 6 files changed, 55 insertions(+), 31 deletions(-)

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

Re: [PATCH v2 0/2] net: thunderx: Support for pass-2 hw features

[David Miller]

From: Sunil Goutham <sunil.kovvuri@gmail.com>
Date: Thu, 10 Dec 2015 13:25:18 +0530

> From: Sunil Goutham <sgoutham@cavium.com>
> 
> This patch set adds support for new features added in pass-2 revision
> of hardware like TSO and count based interrupt coalescing.
> 
> Changes from v1:
> - Addressed comments received regarding boolean bit field changes 
>   by excluding them from this patch. Will submit a seperate
>   patch along with cleanup of unsed field.
> - Got rid of new macro 'VNIC_NAPI_WEIGHT' introduced in 
>   count threshold interrupt patch.

Series applied to net-next, thanks.