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

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

[yadi.brar01 at gmail.com]

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

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

[jonghwa3.lee at samsung.com]

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 at 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.

[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 at 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.

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

[jonghwa3.lee at samsung.com]

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.

[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.

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

[jonghwa3.lee at samsung.com]

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 at 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.

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

[Mark Brown]

On Wed, May 23, 2012 at 02:33:28PM +0900, jonghwa3.lee at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120523/2306419d/attachment.sig>

[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 at 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.

[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.

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

[jonghwa3.lee at samsung.com]

Hi, again.
On 2012? 05? 22? 14:57, yadi.brar01 at 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.

[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 at 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 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

[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 at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Tuesday, December 01, 2015 12:14 PM
> To: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> Sunil.Goutham at 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 ?

[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 ?
>
>
>

[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.

[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.

[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.

[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.

[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.

[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.

[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

[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

[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

[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.

[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

[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

[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 at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Tuesday, December 01, 2015 12:14 PM
> To: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> Sunil.Goutham at 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
>> Goutham
>> Sent: Tuesday, December 01, 2015 12:14 PM
>> To: netdev at vger.kernel.org
>> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> Sunil.Goutham at 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 at 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(-)

[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 at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Monday, December 07, 2015 8:01 AM
> To: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; p.fedin at samsung.com;
> Sunil.Goutham at 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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

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

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Wednesday, December 09, 2015 2:38 PM
> To: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; p.fedin at samsung.com;
> Sunil.Goutham at 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

[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.

[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 at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
>> Goutham
>> Sent: Wednesday, December 09, 2015 2:38 PM
>> To: netdev at vger.kernel.org
>> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; p.fedin at samsung.com;
>> Sunil.Goutham at 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

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

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org] On Behalf Of Sunil
> Goutham
> Sent: Wednesday, December 09, 2015 2:38 PM
> To: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; p.fedin at samsung.com;
> Sunil.Goutham at 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

[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(-)

[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 at vger.kernel.org] On Behalf Of Sunil Goutham
Sent: Thursday, December 10, 2015 10:55 AM
To: netdev at vger.kernel.org
Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; p.fedin at samsung.com; Sunil.Goutham at 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 at vger.kernel.org More
majordomo info at  http://vger.kernel.org/majordomo-info.html

[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.